Re: [lmi] product editor patch

[Greg Chicares]

On 2008-02-08 00:26Z, Vadim Zeitlin wrote:
> On Thu, 07 Feb 2008 21:34:07 +0000 Greg Chicares <address@hidden> wrote:
[...]
> GC> > https://savannah.nongnu.org/patch/index.php?6411
[...]
> GC> /lmi/src/lmi[0]$<../log grep "declared as dllimport: attribute ignored" 
> |wc -l
> GC> 2685
> 
>  We've just spent an hour or so on collective hair pulling with Vaclav but
> finally found the reason for this problem: it's not due to changes in wx
> but due to workhorse,make magic: it uses relaxed gcc warnings for the files
> which use wx code, and in particular -W is not used for wx as it results in
> this warning and there is no way to suppress it with gcc 3.4. However
> workhorse.make uses a heuristics to find the files using wx: it just greps
> for "include.*<wx". But this fails now for tier_view_editor.cpp (and a few
> other files) because Vaclav has corrected the problem indicated by
> "// WX !! why include unused wx headers here" comments as part of his
> changes. So now this files doesn't include any wx headers -- but it still
> uses wx. As you can see, the explanation is rather simple finally but
> arriving at it was quite fun :-/

Thanks for the quick answer. For the moment, I've simply added
a wx header to 'tier_view_editor.hpp' in my local tree; but of
course we'd prefer a permanent solution that'll always work.

>  Anyhow, to solve this I can immediately see 2 solutions:
> 
> 1. Fool-proof but heavy to implement one: use gcc -M to find the list of
>    all dependencies of each file and use this to determine if it uses wx
>    (searching for wx/defs.h in the list of dependencies would be enough)

Heavy, yes, but we already do 'gcc -M' anyway; see below.

> 2. Much simpler but potentially fallible way is to just grep for "wx"
>    (possible improvements: only at the beginning of the word; only outside
>    the comments (which is not so trivial for C comments but should be
>    doable with sed... and certainly doable with cpp if we're ready to run
>    it for all the files just for this))
> 
> What would you prefer? Or maybe you see a better solution?

Not necessarily better...

3. Add a rule to 'test_coding_rules.cpp' to establish a new
invariant on each C++ file's contents, e.g.:
  must match '# *include *<wx/', if there is any 'wx' match
But there may be too many exceptions: a file might have a comment
like "// This standalone file doesn't depend on wx". Let's see:
  /lmi/src/lmi[0]$grep -l 'wx' `grep -L 'include.*<wx' *.?pp` |wc -l
  39
If we could find a way to sidestep the false positives, then this
would be nice because that program runs in about three seconds
and I typically run it many times an hour, so we'd notice any
issues quickly.

But (2) and (3) are fallible, and I'm not sure they can be made
otherwise; and an "invariant" that's true ninety-nine percent of
the time is only a variant, and we've just seen what happens
when we rely on a variant.

4. Add a step to autodependency generation that prints a really
helpful message (e.g., referring to this mailing-list article)
if the 'grep "include.*<wx"' heuristic failed. I think (1) above
is intended to replace that heuristic with an infallible method
that might make builds slower. OTOH, (4) is intended only to
warn when the heuristic fails, but to do so cheaply, as well as
infallibly. Today's issue would have been no big deal if the
diagnostic had been:
  Add '#include <wx/version.h>' to 'tier_view_editor.hpp'!
I think this method can secure the advantage of (1) above with
no speed penalty. We'd change 'autodependency.make':

  MAKEDEPEND_0 = \
    -MMD -MF $*.d0 \

  MAKEDEPEND_1 = \
    $(SED) $(autodependency_sed_commands) < $*.d0 > $*.d; \
+   $(MAGICAL_NEW_COMMAND) \
    $(RM) $*.d0; \

Feel free to give it a try if you're so inclined; I'm going
to be too busy with "work" for the next six days to spend
much time performing any actual *work*.

> GC> We saw something similar before:
> GC>   http://lists.nongnu.org/archive/html/lmi/2006-04/msg00002.html
> 
>  BTW, following this link and your post linked from there (the original URL
> doesn't work, but
> http://readlist.com/lists/lists.sourceforge.net/mingw-users/0/327.html
> is probably the same post)

It is indeed. Old sf.net links are hopelessly broken.

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

I can see how it could be overlooked in maintenance, but couldn't
that be overcome by building with 'gcc -W' in a nightly build so
that any oversight would be caught quickly? BTW, I think this is
an issue with Cygwin gcc-3.4.4, more so than with MinGW.

>  OTOH -Wcast-qual (which currently results in a dozen of warnings in wx
> headers) could probably be worked around. And I haven't seen any
> occurrences of warnings due to -Wredundant-decls so it should be safe to
> enable them for all source files.

I tried adding both, to see what would happen in lmi. As you
say, '-Wcast-qual' would need some changes in wx. As for
'-Wredundant-decls':

In file included from /lmi/src/lmi/main_wx.hpp:43,
                 from /lmi/src/lmi/main_wx.cpp:39:
/opt/lmi/wx-scratch/wxWidgets-2.8.7/include/wx/app.h:613: warning: redundant 
redeclaration of `bool wxYield()' in same scope
/opt/lmi/wx-scratch/wxWidgets-2.8.7/include/wx/utils.h:724: warning: previous 
declaration of `bool wxYield()'
/lmi/src/lmi/main_wx.cpp:106: warning: redundant redeclaration of `Skeleton& 
wxGetApp()' in same scope
/lmi/src/lmi/main_wx.hpp:132: warning: previous declaration of `Skeleton& 
wxGetApp()'
/lmi/src/lmi/main_wx.cpp:187: warning: redundant redeclaration of `int 
wxEntry(HINSTANCE__*, HINSTANCE__*, CHAR*, int)' in same scope
/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 guess the <wx/app.h> versus <wx/utils.h> thing would require
a change in wx itself. But the other warnings look like adding
this warning might unmask some lmi defects, so it's valuable.

BTW, there are three issues with auxiliary targets, two of which
are trivial:

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

(B) 'make check_concinnity' complains about "\n\n\n" in two
files. That's easily fixed. Try this, though:
  /lmi/src/lmi[0]$grep AAAAAAAA *
  lmi.rc:AAAAAAAA ICON "lmi.ico"
  main_wx.cpp:    frame_->SetIcons(wxICON(AAAAAAAA));
  policy_view.xrc:                    AAAAAAAAA
I guess the last one has nothing to do with the first two, and
is just a stray marker that never got erased. It would be nice
to enhance 'make check_concinnity' to find things like that,
maybe by invoking the W3C validator.

(C) 'make install': this command fails:
        @cd $(data_dir); $(bin_dir)/product_files$(EXEEXT)
with the dread diagnostic
  Not all alert function pointers have been set.
which brings us back to this message:

  http://lists.nongnu.org/archive/html/lmi/2005-11/msg00016.html
[...]
| and this matter can wait.

Okay, well, now 's/can/cannot/': I speculate that stronger
internal-consistency checking has exposed a latent logic
error--perhaps in a *proprietary* 'my_tier.?pp' file (see
comment in 'workhorse.make':
  # Files whose names match 'my_%.cpp' are taken as product data files,
  # which are overridden by any customized files found in a special
  # directory.
), so that you wouldn't see it yourself with the public
files--and the attempt to report that problem has exposed yet
another latent logic error. If you want to look into this
before I get the chance, then I bet you could reproduce the
error by adding a line like
  warning() << "You won't see this!" << std::flush;
anywhere that it's sure to be executed. We need to pay close
attention to that 2005-11 discussion, though, because it shows
other places that need fixing, too.

Also, BTW, I think I updated HEAD just after the patch was
prepared, so if you apply it to current HEAD, you may need to
make a manual adjustment for this change:

http://cvs.sv.gnu.org/viewvc/lmi/lmi/illustration_view.cpp?r1=1.72&r2=1.73

where some smart pointer that was apparently overkill is now
a plain stack object that needs 's/->/./'.