= Analysis Report for inkscape =
Platform Unpack Config Build Install WRNS ERRS ------------------------------------------------------------------------ debian x86 0 0 0 0 5 0
Test Status Score Info ------------------------------------------------------------------------ sut-1 ??
== Errors ==
none
== Warnings ==
display/canvas-arena.cpp:91: warning: invalid access to non-static data member display/canvas-arena.cpp:91: warning: (perhaps the `offsetof' macro was used display/sp-canvas.cpp:112: warning: invalid access to non-static data member ` display/sp-canvas.cpp:112: warning: (perhaps the `offsetof' macro was used libnrtype/Layout-TNG-Output.cpp:226: warning: enumeration value `
On Tue, Jul 12, 2005 at 12:56:55AM -0700, Bryce Harrington wrote:
libnrtype/Layout-TNG-Output.cpp:226: warning: enumeration value `
This is in unused code. I have a patch that addresses this by adding some stuff to configure.ac. The reason I haven't committed is that it seems over-the-top to prevent a warning in dead code; I wonder whether we should just remove the function instead.
cyreve, can you comment as to whether you still find this function useful for debugging?
pjrm.
On Tue, 2005-07-12 at 18:22 +1000, Peter Moulder wrote:
This is in unused code. I have a patch that addresses this by adding some stuff to configure.ac. The reason I haven't committed is that it seems over-the-top to prevent a warning in dead code; I wonder whether we should just remove the function instead.
If it's unused, it should be removed. Period. Debugging stuff gets a pass, but only in the short-term.
-mental
This is in unused code. I have a patch that addresses this by adding some stuff to configure.ac. The reason I haven't committed is that it seems over-the-top to prevent a warning in dead code; I wonder whether we should just remove the function instead.
If it's unused, it should be removed. Period. Debugging stuff gets a pass, but only in the short-term.
One of the first things I do when debugging a problem with layout is to turn on this bit of code, so it's still very useful (and will become more so when I start adding features to flowtext again after the release). It is still debugging code, however, and will not be callable in the final build. How do you guys deal with such things? I have read of a disapproval of #iffing out code, and putting it in a separate source file might cause me to accidentally check in a change to the makefile. Also, I do sort of have a vision of using it in some sort of automated regression test. How does that affect your answer?
As for the matter of the warning, the #ifdef was never really meant to be serious, it was just a way to get the file to build again on my machine. Now that win32 is upgraded to gtk2.6, is there anybody left who still does not have that constant?
Richard.
On Tue, 2005-07-12 at 20:34 +0100, Richard Hughes wrote:
One of the first things I do when debugging a problem with layout is to turn on this bit of code, so it's still very useful (and will become more so when I start adding features to flowtext again after the release). It is still debugging code, however, and will not be callable in the final build. How do you guys deal with such things? I have read of a disapproval of #iffing out code, and putting it in a separate source file might cause me to accidentally check in a change to the makefile. Also, I do sort of have a vision of using it in some sort of automated regression test. How does that affect your answer?
My personal two cents:
If it's really that useful for debugging, I would seriously consider making it part of the normal build and adding a runtime switch for it (perhaps via an environment variable, checked once).
Remember, there's no well-defined line between our users and developers, and there probably shouldn't be.
But, the big reason we don't like #ifdefs (or dead code in general) is that they add a tremendous burden both to refactoring and to code comprehension, and that such code inevitably gets broken when other people make changes.
I'm something of a worst-case in that regard; I semi-regularly turn over huge swaths of the codebase to advance long-term design goals. I physically _can't_ review all the code affected in the process.
So, I have to rely on a proof that my changes don't alter the semantics of affected code, double-checked by a fresh compile and a set of functional tests. If your code was part of the affected, but was #ifdefed out, chances are it got missed by my analysis and probably got broken. And either way I won't know because as far as I can tell everything still compiles and works (it's not like I can try every n^2 combination of #defines).
Compiling isn't a proof of correctness, of course, but the compiler can check _some_ things. #ifdefs limit its ability to do even that comprehensively.
[ I do try to take special pains for OS-specific code, but often that means replacing it with something cross-platform as a separate commit. ]
The other big refactorers (notably Peter, who's something of an unsung hero) are not quite as insane. But #ifdefs still make their work a lot harder than it needs to be, and some #ifdef rot is inevitable.
So, I'd say either make it part of the normal build, selectable at runtime, or at least write some trivial unit tests that use it so it gets compile-time exercise that way.
No matter what you do, code behind an interface boundary in a separate file (even if it's just one giant #ifdef) is generally better than having #ifdef blocks sprinkled in among other code, and is more rot-resistant.
Admittedly, sometimes granular #ifdefs are unavoidable. But try to minimize them as much as you can; generally the best way to do that is to do as much as you can in code that is part of the normal build (or unit tests, as a second-best).
Another strategy for some things is to use regular if () with a compile-time constant, instead of #ifdef. If false, the compiler will ensure that the omitted code still compiles, but will (in principle) not include it in its output. It also won't be invisible to analysis tools.
-mental
On a side note re the #ifdefs, how do you handle platform or OS specific code? Makefiles that include or exclude particular versions? Some very specific ifdefs? or isnt anything required?
Craig Scribus Team
Quoting Craig Bradney <cbradney@...242...>:
On a side note re the #ifdefs, how do you handle platform or OS specific code? Makefiles that include or exclude particular versions? Some very specific ifdefs? or isnt anything required?
The platonic ideal (which we don't necessarily achieve) is:
1. write as little platform-specific code as possible
2. agressively use portable library facilities in favor of rolling our own stuff
3. hide the rest behind OS-neutral interfaces (e.g. tables of function pointers or abstract base classes) which won't require #ifdefs in client code
4. make things configurable at run-time, not compile-time
5. do the rest (e.g. calling chosen initialization functions, or extracting data from certain fields) with extremely specific #ifdefs (one or two lines of code each)
-mental
On Wed, Jul 13, 2005 at 01:51:21PM -0400, mental@...3... wrote:
Quoting Craig Bradney <cbradney@...242...>:
On a side note re the #ifdefs, how do you handle platform or OS specific code? Makefiles that include or exclude particular versions? Some very specific ifdefs? or isnt anything required?
The platonic ideal (which we don't necessarily achieve) is:
write as little platform-specific code as possible
agressively use portable library facilities in favor of rolling our own stuff
hide the rest behind OS-neutral interfaces (e.g. tables of function pointers or abstract base classes) which won't require #ifdefs in client code
make things configurable at run-time, not compile-time
do the rest (e.g. calling chosen initialization functions, or extracting data from certain fields) with extremely specific #ifdefs (one or two lines of code each)
Fwiw, it is possible to do testing of compilation of platform-specific code using 'cross-compilers'. I do this for NFSv4 but haven't done it for Inkscape (I figure its a lower priority since it's probably unlikely to generate as much interesting data as other tests we could run).
Bryce
On Wednesday 13 July 2005 20:31, Bryce Harrington wrote:
On Wed, Jul 13, 2005 at 01:51:21PM -0400, mental@...3... wrote:
Quoting Craig Bradney <cbradney@...242...>:
On a side note re the #ifdefs, how do you handle platform or OS specific code? Makefiles that include or exclude particular versions? Some very specific ifdefs? or isnt anything required?
The platonic ideal (which we don't necessarily achieve) is:
write as little platform-specific code as possible
agressively use portable library facilities in favor of rolling our own stuff
hide the rest behind OS-neutral interfaces (e.g. tables of function pointers or abstract base classes) which won't require #ifdefs in client code
make things configurable at run-time, not compile-time
do the rest (e.g. calling chosen initialization functions, or extracting data from certain fields) with extremely specific #ifdefs (one or two lines of code each)
Fwiw, it is possible to do testing of compilation of platform-specific code using 'cross-compilers'. I do this for NFSv4 but haven't done it for Inkscape (I figure its a lower priority since it's probably unlikely to generate as much interesting data as other tests we could run).
Yeah.. assuming you use the same compiler on all platforms, and same library loading routines and signal handling.
I think we will follow #3 a lot.. until the move to Qt4 which is awhile away.
Thanks for the tips.
Craig
On Wed, Jul 13, 2005 at 02:48:18AM -0400, MenTaLguY wrote:
On Tue, 2005-07-12 at 20:34 +0100, Richard Hughes wrote: So, I have to rely on a proof that my changes don't alter the semantics of affected code, double-checked by a fresh compile and a set of functional tests. If your code was part of the affected, but was #ifdefed out, chances are it got missed by my analysis and probably got broken. And either way I won't know because as far as I can tell everything still compiles and works (it's not like I can try every n^2 combination of #defines).
Hmm... You know...
I think your parenthetical was just rhetorical, but actually with our new automated test harness we now we could actually do this on some regular basis (maybe weekly?)
I did a few grep's through the codebase for unique instances of:
5012 #define 1049 #ifdef 785 #ifndef
If someone could narrow that to a list of no more than, say, 20 cases, it would be feasible to automate running as a weekly check.
Think it's worth doing? I agree that using #ifdef's is poor coding practice, but like you say, sometimes it's unavoidable, and I'm thinking that by automating the testing of those areas we might be able to mitigate their dangers. If someone could provide me with a list of defines to test, it would not be at all difficult to set this up. Even if it doesn't turn up many issues, it would give you a bit more peace of mind about your changes.
(I noticed that after only 2-3 reports from the regular builds that folks have gleefully eliminated all issues found, so I think we need some bigger challenges. *grin*)
Also, fwiw, it is fairly easy for us to hook up automated runs of unit tests now, such as running 'make test' or 'make distcheck'. This isn't hooked up right at the moment but it's very high on the todo list. So if anyone is considering making a unit test, the effort will most definitely not go to waste.
Bryce
Quoting Bryce Harrington <bryce@...260...>:
If someone could narrow that to a list of no more than, say, 20 cases, it would be feasible to automate running as a weekly check.
20 test cases would mean roughly ... log2(20) ~= 4.3 ... distinct macros.
(I'm assuming you didn't mean 20 individual macros to test, which would mean testing ... 2^20 = 1048576 ... different permutations)
This is, of course, assuming that the value of the #define isn't itself significant...
A lot of these things are tied to platform feature tests, so we kind of have to ignore those, and some are repeated tests for the same macro, but ... even given that I don't easily forsee being able to pare ~2000 some conditions down to four or five to actually test.
Oh. You know what, I bet a lot of those are header file #include armor. That's the one GOOD use of #ifndef.
Can you subtract the number of header files in the tree from both your #ifndef and #define figures and see what you get?
-mental
On Wed, Jul 13, 2005 at 01:40:34PM -0400, mental@...3... wrote:
Quoting Bryce Harrington <bryce@...260...>:
If someone could narrow that to a list of no more than, say, 20 cases, it would be feasible to automate running as a weekly check.
20 test cases would mean roughly ... log2(20) ~= 4.3 ... distinct macros.
(I'm assuming you didn't mean 20 individual macros to test, which would mean testing ... 2^20 = 1048576 ... different permutations)
What I meant was 20 individual compiles. I think the system could cope with about that many, max. However you'd like to see that broken down in terms of permutations is open.
This is, of course, assuming that the value of the #define isn't itself significant...
A lot of these things are tied to platform feature tests, so we kind of have to ignore those, and some are repeated tests for the same macro, but ... even given that I don't easily forsee being able to pare ~2000 some conditions down to four or five to actually test.
Oh. You know what, I bet a lot of those are header file #include armor. That's the one GOOD use of #ifndef.
Can you subtract the number of header files in the tree from both your #ifndef and #define figures and see what you get?
There are:
652 header files 1053 uniq ifdef's 785 uniq ifndef's
If I do a grep -v on "*_H", that removes a lot of the armor
834 uniq non-armored ifdef 240 uniq non-armored ifndef
Playing around with it a bit more, this seems to remove a lot of the uninteresting clutter:
egrep -hsr "#ifdef|#ifndef" . | egrep -v "cxxtest|ChangeLog|config|m4" \ | egrep "^\s*#" | sed -e 's/#ifn*def //' | grep -iv "_H" \ | egrep -v '__cplusplus|__STDC|GNUC' | sed -e 's/ *$//' \ | sort | uniq | wc -l
This cuts us down from 1838 to 269. Most of the remaining defines appear to be stuff worth reviewing. A good chunk of those 269 defs look pretty crufty, maybe a lot of those could be dropped directly. I'll attach the list below.
Bryce
# __CYGWIN__ APSTUDIO_INVOKED APSTUDIO_READONLY_SYMBOLS ARC_VERBOSE Avoid_Underflow BEZIER_DEBUG BRYCE_FLOATS BR_NO_MACROS BR_THREADS CLAMP CONST COUNT_ALLOCS CROSS_COMPILE CR_DEBUG CXXTEST_MAX_DUMP_SIZE CXXTEST_MOCK_NAMESPACE CXXTEST_USER_VALUE_TRAITS Check_FLT_ROUNDS DATADIR DEBUG DEBUG_MATCH DEBUG_METADATA DEBUG_SCOPE_COUNT DEBUG_SKELETON DEBUG_STREAMS DEBUG_XXXbrendan DEBUG_brendan DEBUG_notbrendan DIR_SEPARATOR DIR_SEPARATOR_2 DUMB_OS_LIKE_WINDOWS DUMP_CHANGE_INFO DYNA_DRAW_VERBOSE EDITLINE ELLIPSE_VERBOSE ENABLE_BINRELOC ENABLE_NLS ENABLE_OOM_TESTING ENTER EXPORT_JS_API Error FALSE FALSE /* Mac standard is lower case false */ FAR FREEBSD_WORKAROUND GC_MARK_DEBUG G_ENABLE_DEBUG G_OS_WIN32 HASHMETER HAS_ECVT HAS_SHGetSpecialFolderLocation HAVE_ATOMIC_DWORD_ACCESS HAVE_BIND_TEXTDOMAIN_CODESET HAVE_FPSETMASK HAVE_GCC_LOOP_BUG HAVE_GTK_WINDOW_FULLSCREEN HAVE_GTK_WINDOW_SET_DEFAULT_ICON_FROM_FILE HAVE_LIBWMF HAVE_LOCALTIME_R HAVE_MALLINFO HAVE_NEW_INTERSECTOR_CODE HAVE_STRUCT_MALLINFO_FORDBLKS HAVE_STRUCT_MALLINFO_FSMBLKS HAVE_STRUCT_MALLINFO_UORDBLKS HAVE_STRUCT_MALLINFO_USMBLKS HAVE_VA_COPY HAVE_VA_LIST_AS_ARRAY HAVE_WATCOM_BUG_2 HAVE_XPCONNECT HPUX HT_ENUMERATE_NEXT /* XXX don't require jshash.h */ HUGE_VAL /* this is the only routine that uses HUGE_VAL */ INFNAN_CHECK INK_DUMP_FILENAME_CONV INK_DUMP_FOPEN IS_LITTLE_ENDIAN Inaccurate_Divide JSDEBUGGER JSDEBUGGER_C_UI JSDEBUGGER_JAVA_UI JSD_LOWLEVEL_SOURCE JS_ARENAMETER JS_ARENA_CONST_ALIGN_MASK JS_ARGUMENT_FORMATTER_DEFINED JS_DHASHMETER JS_DHASH_MIN_SIZE JS_GCMETER JS_PROPERTY_CACHE_METERING JS_STACK_GROWTH_DIRECTION JS_THREADSAFE JS_VERSION LABEL LAZY_STANDARD_CLASSES LIVECONNECT LONG_MAX LT_DLLAZY_OR_NOW Llong Long MALLOC MAX MAX_INTERP_LEVEL METER_PARSENODES METHOD MIN MOZILLA_CLIENT MULTIPLICITY M_E M_LN10 M_LN2 M_LOG10E M_LOG2E M_PI M_SQRT1_2 M_SQRT2 Move NAME_ALL_GC_ROOTS NAN_WORD0 NAN_WORD1 NDEBUG NEWSOS4 NO_IEEE_Scale NR_ARENA_ITEM_DEBUG_CASCADE NR_ARENA_ITEM_VERBOSE NR_DISABLE_CAST_CHECKS NR_USE_GENERIC_RENDERER NR_VERBOSE NR_VERTEX_ALLOC NSPR_LOCK No_leftright OFFSET_VERBOSE OLD_GETTER_SETTER Omit_Private_Memory PANGO_WEIGHT_SEMIBOLD // not available on pango before 1.8 PBM_SETBARCOLOR PBM_SETRANGE32 PERLCONNECT PERL_OBJECT POPT_TABLEEND PRIVATE_MEM RESERVE_ECMA_KEYWORDS RESERVE_JAVA_KEYWORDS RND_PRODQUOT ROUND_BIASED RTLD_GLOBAL SEEN_INKSCAPE_WIDGETS_LAYER_SELECTOR SEEN_INKSCAPE_XML_SP_REPR_EVENT_VECTOR SEEN_IN_DT_COORDSYS SEEN_SP_CONN_END SEEN_SP_CONN_END_PAIR SET_OBJ_INFO SET_SCOPE_INFO SOLARIS SPCS_PREVIEW SPIRAL_DEBUG SPIRAL_VERBOSE SP_DOCUMENT_DEBUG_IDLE SP_FS_VERBOSE SP_GRADIENT_VERBOSE SP_MACROS_SILENT SP_OBJECT_DEBUG SP_OBJECT_DEBUG_CASCADE SP_PS_VERBOSE SP_SS_VERBOSE STRICT_RGBA SUNOS4 SWIGINLINE SWIGINTERN SWIGINTERNSHORT SWIGMODINIT SWIG_COBJECT_TYPES SWIG_LINK_RUNTIME SWIG_TEMPLATE_DISAMBIGUATOR SWIG_TYPE_TABLE Sudden_Underflow TED TESTFTOS TEST_CVTARGS TEST_EXPORT TOO_MUCH_GC TRUE TRUE /* Mac standard is lower case true */ ULLong ULong UNIT_SELECTOR_VERBOSE VERBOSE WIN32 WITH_GNOME_PRINT WITH_GNOME_VFS WITH_GTKSPELL WITH_INKJAR WITH_MMX WITH_MODULES WITH_PERL WITH_PYTHON WITH_UNICODE_ESCAPE_AND_RANGE WORDS_BIGENDIAN XP_BEOS XP_MAC XP_MACOSX XP_MAC_MPW XP_OS2 XP_UNIX XP_WIN XP_WIN16 XXX _CXXTEST_FACTOR _CXXTEST_OLD_STD _CXXTEST_OLD_TEMPLATE_SYNTAX _CXXTEST_PARTIAL_TEMPLATE_SPECIALIZATION _IEEE_LIBM _MSC_VER _MSC_VER // Visual C++ _POSIX_MODE _PREFIX_C_ _PTRDIFF_T_DEFINED _REENTRANT _SCALB_INT _SEQUENT_ _SVID3_MODE _SVID_GETTOD /* Defined only on Solaris, see Solaris <sys/types.h> */ _USE_WRITE _WIN32 _WINDLL _WINDOWS _XOPEN_MODE _XP_Core_ __BORLANDC__ __DMC__ // Digital Mars __GNU_LIBRARY__ __LITTLE_ENDIAN __MWERKS__ __NEWVALID /* special setup for Sun test regime */ __SP_DESKTOP_C__ __SP_SELECTION_CHEMISTRY_C__ __WINDOWS_386__ apply arena_item_tile_cache convert do_close do_open faster_flatten form invert js_cpucfg___ lalaWITH_MODULES list my_alpha_ligne my_avl my_bit_ligne my_defs my_font_factory my_math my_math_seg my_path my_raster_font my_shape my_text_wrapper pTHX_ ref scalar signal test_glyph_liv tile_cache_stats va_start vform with_splotch_killer yyerror yylex yyparse
participants (6)
-
unknown＠example.com -
Bryce Harrington -
Craig Bradney -
MenTaLguY -
Peter Moulder -
Richard Hughes