Inkscape

Bug #988601
Comment #162

Comment 162 for bug 988601

Revision history for this message

David Mathog (mathog) wrote on 2013-03-19:

#162

Re: 158

Thanks, gcc didn't pick up on the first two. I have fixed them but will wait for the next major patch to release them since neither actually affects anything. The first is in an unused (by Inkscape) function, and the second is just an extra set of
parenthesis around a value, which is harmless.

The "cubic = cubic" line in emf-print.cpp and wmf-print.cpp dates back to before I started working on the code. I can't imagine what the original purpose was , but it will be removed and if nothing breaks, it will stay out.

As for the others...
It helps to keep in mind that compilers are free to warn about anything they want, including things that the programmer intended to do knowing full well what the consequences might be.

The line "torev = torev" was added to shut up warnings on gcc that "torev" was declared but not used. That was true but intentional. "torev" was at the end of the parameter list on a whole series of function calls and I thought it better to maintain the style even if a small number of them did not actually use (in the current version) that parameter. The "torev = torev" construct silenced gcc. Apparently clang is not amused by it.

The rest of the warnings seem to this form, or a slight variant thereof:

../../src/extension/internal/emf-print.cpp:426:27: warning: cast from 'char *' to 'PU_ENHMETARECORD' (aka 'U_ENHMETARECORD *') increases required alignment from 1 to 4 [-Wcast-align]

where what the compiler is warning about was intended and is the correct cast. (If it was not the program would not work.) Note that what the compiler is warning about is not the cast itself, but the change in required alignment that results from it (-Wcast-align). The programmer is aware of the alignment issues (which were fairly horrendous for WMF, where the records are 2N bytes long, leading to embedded int32_t 's becoming misaligned) and these alignment issues are, as best as I can tell, all handled. So, changing the form of the cast to the longer C++ syntax should not eliminate this warning, since any form of cast that accomplishes the same thing is going to change the alignment requirements. I don't use clang, but if ~suv wants to do the experiment, change any one of these C style casts to the C++ syntax and recompile. The associated clang warning should still appear.

So, re 161, these warnings do not appear to indicate any problem with the C style casts.

Re: 160. The way the variables are arranged dates way back and I have just been sticking with the way it was done in the beginning. By global are you referring to the few up at the top of the .cpp files, or the ones in the .h that follow for instance here:

class PrintWmf : public Inkscape::Extension::Implementation::Implementation
{

? Moving the few global variables declared in the .cpp in with the others in the .h is on my todo list - I just keep forgetting "todo" it!

Re: 158

Thanks, gcc didn't pick up on the first two.  I have fixed them but will wait for the next major patch to release them since neither actually affects anything.  The first is in an unused (by Inkscape) function, and the second is just an extra set of 
parenthesis around a value, which is harmless.

The "cubic = cubic" line in emf-print.cpp and wmf-print.cpp dates back to before I started working on the code.  I can't imagine what the original purpose was ,  but it will be removed and if nothing breaks, it will stay out.

The line "torev = torev" was added to shut up warnings on gcc that "torev" was declared but not used.   That was true but intentional.  "torev" was at the end of the parameter list on a whole series of function calls and I thought it better to maintain the style even if a small number of them did not actually use (in the current version) that parameter.  The "torev = torev" construct silenced gcc.  Apparently clang is not amused by it.

The rest of the warnings seem to this form, or a slight variant thereof:

../../src/extension/internal/emf-print.cpp:426:27: warning: cast from 'char *' to 'PU_ENHMETARECORD' (aka 'U_ENHMETARECORD *') increases required alignment from 1 to 4 [-Wcast-align]

where what the compiler is warning about was intended and is the correct cast.   (If it was not the program would not work.)  Note that what the compiler is warning about is not the cast itself, but the change in required alignment that results from it (-Wcast-align).   The programmer is aware of the alignment issues (which were fairly horrendous for WMF, where the records are 2N bytes long, leading to embedded int32_t 's becoming misaligned)  and these alignment issues are, as best as I can tell, all handled.   So, changing the form of the cast to the longer C++ syntax should not eliminate this warning, since any form of cast that accomplishes the same thing is going to change the alignment requirements.  I don't use clang, but if ~suv wants to do the experiment, change any one of these C style casts to the C++ syntax and recompile.  The associated clang warning should still appear.

So, re 161, these warnings do not appear to indicate any problem with the C style casts.

Re: 160.  The way the variables are arranged dates way back and I have just been sticking with the way it was done in the beginning.   By global are you referring to the few up at the top of the .cpp files, or the ones in the .h that follow for instance here:

class PrintWmf : public Inkscape::Extension::Implementation::Implementation
{

?  Moving the few global variables declared in the .cpp in with the others in the .h is on my todo list - I just keep forgetting "todo" it!