Re: [lmi] PATCH: tests build fixes for clang

[Vadim Zeitlin]

On Sun, 7 Mar 2021 15:25:57 +0000 Greg Chicares <gchicares@sbcglobal.net> wrote:

GC> On 3/7/21 1:10 PM, Vadim Zeitlin wrote:
GC> > 
GC> >  I'd like to submit a series of small fixes to the tests allowing to build
GC> > them with clang, please see 
https://github.com/let-me-illustrate/lmi/pull/173
GC> 
GC> I glanced quickly at the commit messages, and each seems to be a
GC> valuable improvement. Of course, the larger goal of supporting
GC> clang is most valuable.

 Thanks!

GC> >  One thing I did not do, although I think it would be worth doing, was to
GC> > define LMI_GCC and LMI_CLANG macros in config.hpp. The reason I'd like to
GC> > do this is that the number of occurrences of
GC> > 
GC> >   #if defined __GNUC__ && !defined __clang__
GC> > 
GC> > tests increases from 4 to 10 with this PR and it risks continuing growing
GC> > in the future, so I'd prefer to write this as shorter and clearer
GC> > 
GC> >   #if defined LMI_GCC
GC> > 
GC> > instead. If you agree that it would be a good idea, please let me know and
GC> > I'll add a commit doing this to this PR (or create another PR with it, as
GC> > you prefer).
GC> 
GC> Sounds good. Would you prefer to do that before I merge PR 173,
GC> or should I merge PR 173 as it stands now?

 I've just added another commit (cce367c18) to this PR with this change.
You would have to re-run git-fetch again if you had already done it before
merging/cherry-picking.

GC> >  Just for completeness, another possibility I've considered was to define
GC> > the existing LMI_GCC_VERSION only for "genuine" gcc, but I don't think 
it's
GC> > a good idea because:
GC> > 
GC> > 1. The existing uses of LMI_GCC_VERSION should be removed[*] in the first
GC> >    place as they're very obsolete (some check for gcc 3.2!).
GC> At a quick glance, it looks like at least one might guard some
GC> code that revealed a compiler defect. Sometimes a defect seen
GC> in one compiler in one year later appears in a later year with
GC> a different compiler, so I'm pretty conservative about removing
GC> conditional workarounds. But I'll take a look at every instance.

 It would be even nicer to remove it now because defining it for clang
doesn't make any sense (it doesn't use the same version numbers as gcc) but
doing it only for LMI_GCC doesn't work without further changes as it is
assumed to be defined whenever __GNUC__ is, so it would need to be replaced
with LMI_GCC everywhere where LMI_GCC_VERSION is used. And just not using
it at all would be simpler and, IMHO, better because gcc 8 (and soon 10 or
11) that we use is not at all the same compiler as gcc 3 or 4 anyhow. BTW,
if you do this, please also remove or update the check for gcc 2 (!) in
config.hpp which is, of course, even more thoroughly obsolete.

 As for LMI_GCC_VERSION definition itself, it could still remain, if you
think it could be useful in the future, of course, but then it should be
guarded by LMI_GCC and not __GNUC__ as now.

 Thanks again,
VZ

pgpybkGZF19V0.pgp
Description: PGP signature