Re: [lmi] PCH

[Greg Chicares]

On 2016-06-13 16:21, Vadim Zeitlin wrote:
> On Mon, 13 Jun 2016 15:38:02 +0000 Greg Chicares <address@hidden> wrote:
[...]
> 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).

I've committed the changes to the other files.

> 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.

Done.

> 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.

Okay, I read "removed it from the other targets (such as product_files)"
as suggesting that product_files$(EXEEXT) would no longer link that object.

>  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?

Actually, I had thought of proposing that as an alternative less violent
than what I thought you were suggesting. Let me give that some thought.

> 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.

It's really nice that git makes it so easy to explain things in the commit
message. I was in the habit of cramming explanations into one line for the
old svn 'ChangeLog' (I could add paragraphs there, but then I'd have to
commit the change log along with the source-code modifications, and the
log is so large that updating it on the central repository was painful).
This improvement in convenience is comparable to replacing SMS with email.
But I'm still in the process of adjusting my habits, so, although I felt
sure you had explained that in detail, a diligent search of my email
archives failed to find it. Thanks for repeating the thorough explanation.