Re[2]: [lmi] product editor patch

[Vadim Zeitlin]

On Fri, 08 Feb 2008 05:18:22 +0000 Greg Chicares <address@hidden> wrote:

[for reference I had proposed:]
GC> > 1. Fool-proof but heavy to implement one: use gcc -M
...
GC> > 2. Much simpler but potentially fallible way is to just grep for "wx"

GC> 3. Add a rule to 'test_coding_rules.cpp' to establish a new
GC> invariant on each C++ file's contents, e.g.:
GC>   must match '# *include *<wx/', if there is any 'wx' match

 But is it really a good idea to include headers unnecessarily (and
somewhat arbitrarily) just to make the makefile work? IMO it isn't. I can
agree with coding guidelines stating that each file should start with the
inclusion of <wx/wxprec.h> because this is necessary to make precompiled
headers work and there is no way around it (BTW, it might be a good idea to
think about using precompiled headers in LMI, this should significantly
speed up compilation). But including otherwise useless header just to make
a makefile happy seems singularly unappealing to me.

GC> But there may be too many exceptions: a file might have a comment
GC> like "// This standalone file doesn't depend on wx". Let's see:
GC>   /lmi/src/lmi[0]$grep -l 'wx' `grep -L 'include.*<wx' *.?pp` |wc -l
GC>   39

 Well, we're interested only in .cpp files here, which reduces this to 15.
And with the exception of wx_new[_test].cpp all of them which have "wx"
outside of a comment do use wx. So (2) should work for us right now with
just one (or two, counting the test) false positives.

GC> 4. Add a step to autodependency generation that prints a really
GC> helpful message (e.g., referring to this mailing-list article)
GC> if the 'grep "include.*<wx"' heuristic failed.

 This would be definitely useful to diagnose the problem but I'd still like
a better way to fix it once it's diagnosed...

 BTW, another simple but possibly good enough heuristic would be

5. Grep for '# *include +<wx/' in `basename $i .cpp`.hpp too.

This seems to work fine with the current sources -- but isn't 100%
fool-proof neither, of course.



GC> > I see that my explanation about the need of
GC> > dllimport for inline functions was apparently wrong. However it still
GC> > wouldn't be easy to fix because the warning appears for the inline methods
GC> > of a class using WXDLLIMPEXP declaration. And while some classes (such as
GC> > wxCharBuffer which is the first one giving this warning) are fully inline,
GC> > others have both inline and non-inline methods and if we don't use
GC> > WXDLLIMPEXP on the class declaration we need to use it for all non-inline
GC> > methods which is not only annoying but also extremely error-prone as it's
GC> > awfully easy to forget it on a method. So I'm afraid we have no choice but
GC> > to continue to avoid -W with wx code.
GC> 
GC> I can see how it could be overlooked in maintenance, but couldn't
GC> that be overcome by building with 'gcc -W' in a nightly build so
GC> that any oversight would be caught quickly?

 Sorry, I don't see how could it help: -W will complain about the inline
functions in a class with WXDLLIMPEXP. It won't say anything about
non-inline functions of a class without WXDLLIMPEXP not marked with
WXDLLIMPEXP themselves (which will make them unusable when using wx as a
DLL).

GC> BTW, I think this is an issue with Cygwin gcc-3.4.4, more so than with
GC> MinGW.

 I don't see any difference between 

        gcc version 3.4.4 (mingw special)

and

        gcc version 3.4.4 (cygming special, gdc 0.12, using dmd 0.125)

which I have here in this respect.


GC> >  OTOH -Wcast-qual (which currently results in a dozen of warnings in wx
GC> > headers) could probably be worked around. And I haven't seen any
GC> > occurrences of warnings due to -Wredundant-decls so it should be safe to
GC> > enable them for all source files.
GC> 
GC> I tried adding both, to see what would happen in lmi. As you
GC> say, '-Wcast-qual' would need some changes in wx. As for
GC> '-Wredundant-decls':
GC> 
GC> In file included from /lmi/src/lmi/main_wx.hpp:43,
GC>                  from /lmi/src/lmi/main_wx.cpp:39:
GC> /opt/lmi/wx-scratch/wxWidgets-2.8.7/include/wx/app.h:613: warning: 
redundant redeclaration of `bool wxYield()' in same scope
GC> /opt/lmi/wx-scratch/wxWidgets-2.8.7/include/wx/utils.h:724: warning: 
previous declaration of `bool wxYield()'

 This should be fixed in the latest wx svn (both trunk and 2.8 branch).

GC> /lmi/src/lmi/main_wx.cpp:106: warning: redundant redeclaration of 
`Skeleton& wxGetApp()' in same scope
GC> /lmi/src/lmi/main_wx.hpp:132: warning: previous declaration of `Skeleton& 
wxGetApp()'

 This one is nice: it's due to the fact that IMPLEMENT_APP_NO_MAIN()
does DECLARE_APP() internally and, of course, normally there should be
no need for it as IMPLEMENT_APP_NO_MAIN() defines the wxGetApp()
function which DECLARE_APP() only declares. But, as I've just found using
svn annotate, I did this change myself to fix icc (Intel compiler) warning
about "defining an external function without prior declaration". So fixing
warning for one compiler introduced it for the other...

 I'm honestly unsure about what to do here. We could only use DECLARE_APP()
inside IMPLEMENT_APP_NO_MAIN() for icc (or maybe not do it for gcc) but
this complicates the code and risks introducing bugs resulting from
different behaviours when using different compilers and all for a very
uncertain gain as -Wredundant-decls looks like a rather useless warning to
me: does it ever indicate a real problem anyhow? OTOH LMI could work around
it by #defin'ing some LMI_MAIN symbol in main_wx.cpp and wrapping
DECLARE_APP() in main_wx.hpp in "#ifndef LMI_MAIN". Would this be
sufficient?

GC> /lmi/src/lmi/main_wx.cpp:187: warning: redundant redeclaration of `int 
wxEntry(HINSTANCE__*, HINSTANCE__*, CHAR*, int)' in same scope
GC> /opt/lmi/wx-scratch/wxWidgets-2.8.7/include/wx/msw/app.h:110: warning: 
previous declaration of `int wxEntry(HINSTANCE__*, HINSTANCE__*, char*, int)'

 I don't really understand what's going on here, in particular why does LMI
define its own WinMain(). But wxEntry declaration does seem to be redundant
here.


GC> BTW, there are three issues with auxiliary targets, two of which
GC> are trivial:
GC> 
GC> (A) 'make check_concinnity' complains about new '.xpm' files.
GC> I can fix that with
GC>   sed -e's/static const unsigned char \*/static char const\*/'
GC> if there's no objection. If you feel that 'unsigned char' is
GC> better, just tell me and I can change all the old '.xpm' files;
GC> I just want them all to follow the same rule.

 Maybe Vaclav had some reason to use unsigned char but I really don't know
of any... IMO it doesn't matter one way or the other (as XPM only uses 7
bit ASCII) but plain char is better because much more standard.

GC> (B) 'make check_concinnity' complains about "\n\n\n" in two
GC> files. That's easily fixed. Try this, though:
GC>   /lmi/src/lmi[0]$grep AAAAAAAA *
GC>   lmi.rc:AAAAAAAA ICON "lmi.ico"
GC>   main_wx.cpp:    frame_->SetIcons(wxICON(AAAAAAAA));
GC>   policy_view.xrc:                    AAAAAAAAA

 Oops.

GC> I guess the last one has nothing to do with the first two, and
GC> is just a stray marker that never got erased. It would be nice
GC> to enhance 'make check_concinnity' to find things like that,
GC> maybe by invoking the W3C validator.

 Yes, this would definitely make sense.

GC> (C) 'make install': this command fails:
GC>     @cd $(data_dir); $(bin_dir)/product_files$(EXEEXT)
GC> with the dread diagnostic
GC>   Not all alert function pointers have been set.
...
GC> Also, BTW, I think I updated HEAD just after the patch was
GC> prepared, so if you apply it to current HEAD, you may need to
GC> make a manual adjustment for this change:
GC> 
GC> http://cvs.sv.gnu.org/viewvc/lmi/lmi/illustration_view.cpp?r1=1.72&r2=1.73
GC> 
GC> where some smart pointer that was apparently overkill is now
GC> a plain stack object that needs 's/->/./'.

 I'm sorry, I have to leave now so I don't have time to look at this
immediately but I'll do it later today if Vaclav doesn't beat me to it.

 Regards,
VZ