Re: [lmi] Fixing build with clang 7 and 64-bit Linux

[Greg Chicares]

On 2018-11-02 16:39, Vadim Zeitlin wrote:
> 
>  I wanted to build lmi with clang to test some changes and it turns out
> there are quite a few of new errors and warnings (which, of course, are
> also fatal due to the use of -Werror) due to the changes done since the
> last time I tested clang build (which must have been a couple of months
> ago, apparently).

[...snip list of diagnostics]

>  The first two errors, about the unused private fields, actually do also
> look straightforward to me and I'd just remove these member variables as
> well as the corresponding SequenceParser ctor arguments. But, AFAIR, the
> last time this warning came up, you preferred to suppress it rather than
> remove the unused fields.

Yes. It came up for exactly these two data members, and I worked around
the clang diagnostic; but later I removed the workaround, thinking that
it was intended for some obsolete compiler. I've re-added the workaround.

>  As for the rest of the errors, they're all due to lmi::ssize() returning
> ssize_t (a.k.a. long) and not int, and so are due to building on a 64-bit
> Linux system rather than using clang. Strangely enough, gcc8 doesn't mind
> initializing int fields with long values, even though I think this should
> be forbidden when using C++11 initialization. But it gives the same error
> about min() ambiguity as well (just with a much longer error message). So
> IMO the solution to them is to make ssize() return int, as was previously
> discussed in e.g. the beginning of this message:
> 
>       http://lists.nongnu.org/archive/html/lmi/2018-08/msg00031.html

In that message I said:

| Yes, it seems that
|   using ssize_t = int;
| is the best way. Two out of three functions in 'ssize_lmi.hpp' already
| use bourn_cast, so the documented precondition ought to be enforced for
| a modified ssize_t in those cases; I'll want to examine the unit test
| to make sure that's covered. I'm not sure the array[n] function is
| robust enough--maybe it should be changed thus:
| -    return {n};
| -    return bourn_cast<ssize_t>(n);
| I'll have to look through old emails and commit messages, because I'm
| pretty sure I documented some reason for not doing that originally.

"documented some reason": git show d74e38ed a80dbb6f

Briefly stated, the reason is that bourn_cast<>() gives a runtime error,
where 'return {n}' gives a compile-time error, and the latter is better.
There's an experimental framework in the unit test for exploring this
behavior; looking at its documentation again, I wonder whether gcc is
really deducing the type correctly; maybe clang will do something
different.

Incidentally...

https://lists.nongnu.org/archive/html/lmi/2018-06/msg00017.html
| instead of 'int', we could use this (as in 'ssize_lmi.hpp'):
|   using ssize_t = std::make_signed_t<std::size_t>;
| or Herb Sutter's recommendation:
|   https://github.com/isocpp/CppCoreGuidelines/pull/1115
|   namespace gsl { using index = ptrdiff_t; }

Instead, I've embraced 'int' as the one and only universal integer
type for the twenty-first century; lmi doesn't need arrays so big
that 'int' can't index them.

> So the question is whether I could make this change or if you'd prefer to
> do it yourself, in which case I'll have to temporarily abandon my efforts
> to fix clang build? I'd prefer to fix it and, ideally, set CI build up on
> GitHub, so that both clang and gcc (and maybe even MSVS?) builds could be
> automatically tested all the time but it's, of course, not very urgent.

I've done it, and will push soon. CI with other compilers is a useful
complement to my habit of running all tests before any push.