Re: [lmi] PCH

[Vadim Zeitlin]

On Mon, 13 Jun 2016 15:38:02 +0000 Greg Chicares <address@hidden> wrote:

GC> I personally don't care which PCH file each TU includes, because I'm not
GC> using PCH. But this part of the patch doesn't just change PCH:
GC> 
GC> 
https://github.com/vadz/lmi/commit/a66c3dd7caffa1bfb3c363fbe18d8e860a9c168e.patch
GC> ...
GC> diff --git a/tier_view.cpp b/tier_view.cpp
GC> index f3fccd8..103681d 100644
GC> --- a/tier_view.cpp
GC> +++ b/tier_view.cpp
GC> @@ -25,6 +25,7 @@
GC> 
GC>  #include "multidimgrid_any.hpp"
GC>  #include "multidimgrid_tools.hpp"
GC> +#include "multidimgrid_safe.tpp"
GC>  #include "safely_dereference_as.hpp"
GC>  #include "stratified_charges.hpp"
GC>  #include "stratified_charges.xpp"
GC> 
GC> Why is that needed? Has it always been wrong?

 Sorry, this was included in this PR by mistake. I planned to open a
separate PR for this change (which is incomplete, as the inclusion of
multidimgrid_safe.tpp should be removed from database_view.cpp) and somehow
accidentally put it into this one, please disregard it and I'll update the
PR soon (after resolving the issue with main_common).

GC> BTW, in commit ee6c0230a33cbf23da6c56faac72254281804a09 I changed the
GC> way 'workhorse.make' determined which objects depend on wx (for the
GC> purpose of suppressing '-Wcast-qual'). It seemed that inclusion of
GC> the wx-specific PCH header was a better way of determining that. Now,
GC> however, we're including that PCH header in some files that actually
GC> don't depend on wx, and those files shouldn't be exempted from that
GC> extra warning--so I'll revert that modification.

 I think ideal would be to just get rid of wx_dependent_objects entirely
and make sure that wxWidgets can be compiled with -Wcast-qual. I see that
there are thousands of these warnings right now, but most of them seem to
come from wx/dynarray.h, so maybe fixing them is not as hopeless as it
seems. If you'd like, I could try to at least look at it and maybe even
actually do it.

 But until this is done, it looks like it would indeed be better to revert
this change, as it prevents some files from being compiled with -Wcast-qual
even though they do not produce any such warnings.


GC> >  I had a small problem with just one file, main_common.cpp, which is also
GC> > part of libskeleton, and so would normally need to include pchfile_wx.hpp,
GC> > but this didn't seem to be appropriate to do for this file as it's also
GC> > used as part of other targets (e.g. product_files) which don't have
GC> > anything to do with wxWidgets. After hesitating for some time, I just 
moved
GC> > this file from libskeleton project to liblmi one and removed it from the
GC> > other targets (such as product_files) and everything builds perfectly for
GC> > me. I wonder if this shouldn't be also done in objects.make, this file 
just
GC> > doesn't seem to belong to libskeleton, unless I'm missing something.
GC> 
GC> I disagree with this change. There are several 'main*.cpp' files, and
GC> code that is common to all has been factored into 'main_common.cpp'.
GC> This change would remove that common code from non-wx binaries: for
GC> example, they would no longer call set_terminate().

 Sorry, I don't understand this at all, I must have again badly explained
what I did. All the targets that link with main_common.o currently still
continue to link with it, by definition non-wx targets don't link with
(wx-dependent) libskeleton anyhow, so they're not affected at all.

 Again, all I suggest doing is this:
---------------------------------- >8 --------------------------------------
diff --git a/objects.make b/objects.make
index c39baff..7972ae7 100644
--- a/objects.make
+++ b/objects.make
@@ -296,6 +296,7 @@ lmi_common_objects := \
   ihs_mortal.o \
   ihs_server7702.o \
   lmi.o \
+  main_common.o \
   md5.o \
   mec_input.o \
   mec_server.o \
@@ -327,7 +328,6 @@ skeleton_objects := \
   illustration_document.o \
   illustration_view.o \
   input_sequence_entry.o \
-  main_common.o \
   mec_document.o \
   mec_view.o \
   msw_workarounds.o \
---------------------------------- >8 --------------------------------------

 What exactly would it break?


GC> What problem are you trying to solve here--something to do with PCH?

 Yes, as I wrote in the commit message:

VZ> All files compiled together as part of the same target must use the
VZ> same precompiled header and as these files are part of libskeleton,
VZ> together with other files that include pchfile_wx.hpp because they
VZ> use wxWidgets, they need to also include this header and not just
VZ> pchfile.hpp as well, even if they don't use any GUI features.

 As long as main_common.cpp is part of the skeleton project, it needs to
use pchfile_wx.hpp, but knowing that it is already used in the targets not
depending on wx, this doesn't look right. And the best solution just seems
to be to remove this non-wx-dependent file from libskeleton. What am I
missing here?

 Thanks,
VZ