Re: Details on some bugs in PenguinFile

[Christian Reiniger <warewolf@mayn.de>]

Bjarke Hammersholt Roune wrote:

NOTE:
I'm now committing a new PFile version (with the VFS stuff in place, but
without the Pak writing code and many API functions).
It compiles here, but that's it.
Please note that some files have been added and others removed.


>I assumed this wouldn't be very interesting for anybody but you, so I
>didn't send it to the list.

Well, at least the ppFatalError () stuff would be interesting to Adrian, as
he wrote most of it ;)

>**** const correctness ****
>cannot convert from 'const char *' to 'char *'
>
>ppf_GlobalData.cpp, line 56, ppf_TGlobalData::ppf_TGlobalData()
>ppFatalError ("Tried to instantiate ppf_TGlobalData twice");
>-
>It took me **QUITE** a while to figure out what the problem here was.
>
>Here's the definition of ppFatalError: (why isn't it in all caps and
>underscores as all other preprocessor symbols?)

Because (1) it behaves like a normal function and its nature as macro is
pretty irrelevant and (2) because it is used all over the code (together
with ppDebugn (), ppWarning (), ppInternalError ()) and making it all caps
would make the code harder to read.

>#define ppFatalError (
>     *_pp_db_lineno_loc()=__LINE__,
>     *_pp_db_file_loc()=__FILE__,
>     *_pp_db_func_loc()=_PP_FUNCTION_NAME,
>      _ppFatalError
>)
>
>the problem is with the symbol _PP_FUNCTION_NAME. Here is the definition of
>this symbol:
>
>#ifdef __GNUC__
>  #define _PP_FUNCTION_NAME __PRETTY_FUNCTION__
>#else
>  extern const char* _ppNoFuncNameMsg;
>  #define _PP_FUNCTION_NAME _ppNoFuncNameMsg;
>#endif
>
>The value yielded by _PP_FUNCTION_NAME must NOT be const. __LINE__ and

In contrary - it *should* be const (to indicate that it is not modified by
ppFatalError ())
The problem is rather that _pp_db_func_loc () is of type char ** (instead
of const char **)

>__FILE__ is not const either. Actually, this code:

???????
__FILE__ is evaluated by the preprocessor into a string constant. Ever
tried to change your current source file name at runtime?

>char* p1 = __FILE__;
>char* p2 = __FILE__;
>*p2 = 'Q';
>cout << p1 << endl;
>cout << p2 << endl;
>
>Will not only compile without problems, but it will also give this output:
>(assuming the file is c:\myfile.cpp, of course)
>
>c:\myfile.cpp
>Q:\myfile.cpp

Then your compiler really has a problem.

>So, p1 and p2 points to different storage, and that's why the __FILE__ can
>yield non-const values safely.

Then the preprocessor would need to change
-----
char *p1 = __FILE__;
-----
to something like this:
-----
#include <string.h>
char *p1 = (char *) malloc (strlen ("thisfilesname"));
strcpy (p1, "thisfilesname");
-----

Sorry, that's just too crazy (on the other hand - it would explain the
memory leaks in MS products ... ;)

>Ok, that was a side note. Anyway, _ppNoFuncNameMsg must de declared to be
>of type char* rather than const char*. I do think it would be a good idea

No. _db_file_loc () & Co have to be changed.

>to initialise it to "", "not supported", "unknown" or something like that
>aswell. That way, no alteration would have to be done to _ppFatalError.

It is initialized to "Unknown Function" (in src/ppUtils.cc)

>Also, the fact that _PP_FUNCTION_NAME was defined as _ppNoFuncNameMsg;
>generated several syntax errors, as there should not be a ";" there. So, to

Ok, fixing that.

>**** double-initialization ****
>'i' : redefinition; multiple initialization
>
>ppf_GlobalData.cc, line 67, ppf_TGlobalData::ppf_TGlobalData()
>for (int i=0 ; i < ppfAM_numvals ; i++)
>
>ppf_GlobalData.cpp, line 93, ppf_TGlobalData::~ppf_TGlobalData()
>for (int i=0 ; i < ppfAM_numvals ; i++)
>-
>Some compilers does not have a problem with this. However, some do, and mine
>is one of them. In cases like this, declare the variable i outside the for

Ugh. Poor guy.

>statement and just initialize it there if you are going to use it more than
>once withind the same scope. That should work for everyone.

STOP - wait a minute. i is *once* initialized in
ppf_TGlobalData::ppf_TGlobalData (i.e. the CONstructor) and *once* in
ppf_TGlobalData::~ppf_TGlobalData  (i.e. the DEstructor) - that's two
different methods!!!!

Do you really want to say that MSVC has a problem with that?????

>**** const correctness ****
>'delete' : cannot convert parameter 1 from 'const char *' to 'void *'
>
>The DirTreePath member of ppf_TGlobalData is deleted on line 98 and 152 in
>ppf_GlobalData.cc.

ppf_GlobalData.cc has been 90% rewritten in the current version. Can you
recheck it?

>The filename member of ppf_PakFile is deleted on line 63 and 69, 73 and 94
>in ppf_PakFile.cc.

Corrected.

>Btw, how would we "merge" our different versions of the code?

Quite simple - we'd take both versions, run "diff" over them and copy the
changes around. Should be quite easy as 99% of the changes won't overlap.


	Christian
--

Drive A: not responding...Formatting C: instead