Re: [lmi] clang 10/C++20 build fixes

[Vadim Zeitlin]

On Fri, 3 Apr 2020 22:53:50 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2020-03-25 15:22, Vadim Zeitlin wrote:
GC> > 
GC> >  I've created several new PRs fixing lmi build with clang 10 including two
GC> > which are more general and apply to any C++20 compilers. They're, in no
GC> > particular order:
GC> 
GC> All pushed, in the order given.

 Thanks a lot! I've closed the corresponding PRs.

GC> >   Incidentally, this PR also suppresses a warning
GC> >   which prevented bourn_cast unit test from compiling with clang.
GC> 
GC> I thought clang had some way of recognizing gcc pragmata (such as had 
already
GC> been written in that file). Indeed:
GC> 
GC> https://clang.llvm.org/docs/UsersManual.html
GC> |
GC> | The following example code will tell Clang or GCC to ignore the -Wall 
warnings:
GC> |
GC> | #pragma GCC diagnostic ignored "-Wall"
GC> 
GC> Is it really necessary to suppress the gcc and clang warnings separately
GC> in this case?

 Unfortunately, yes, because "-Wimplicit-int-float-conversion" is
clang-specific and doesn't exist in gcc (yet?), so it has to be suppressed
with a clang-specific pragma. I agree that it's annoying that some pragmas
work for both compilers, while others work only for exactly one of them,
but I'm afraid that this is only going to get worse with time, not better.
Unless the common sense triumphs and they coordinates their development and
new warning names for the greater good... but this is a subject of another
discussion, so I won't go into it here.

GC> > - https://github.com/vadz/lmi/pull/139 Miscellaneous clang 10 fixes
GC> 
GC> Here:
GC>   commit cc1abb51b  Add missing stream headers
GC> can you help me see why the former code below was not strictly conforming?
GC> 
GC>   grep '[io]stream' rate_table.hpp
GC>       database(std::istream& index_is, std::shared_ptr<std::istream> 
data_is);
GC>       void save(std::ostream& index_os, std::ostream& data_os);
GC>   inline std::ostream& operator<<(std::ostream& os, table::Number const& 
number)
GC> 
GC> AFAICT, both std::istream and std::ostream are mentioned either
GC>  - by reference, or
GC>  - in the template parameter of std::shared_ptr
GC> so I thought the ideal fix would be to add
GC>   #include <iosfwd>
GC> to the header,

 No, this is insufficient, std::ostream is actually used in the inline
implementation of operator<<() overload for table::Number.

GC> and both of these
GC>   #include <istream>
GC>   #include <ostream>
GC> to the associated TU, viz., 'rate_table.cpp'.
GC> 
GC> Similarly, for 'ce_product_name.?pp', I should think
GC> +  #include <iosfwd>
GC> for '.hpp', and
GC> +  #include <istream>
GC> +  #include <ostream>
GC> for '.cpp' would be ideal.

 It would be more clear, but clearly <ostream> was already included from
somewhere else, and I was trying to keep my changes to the minimum.

GC> Combining those points, and also removing a repeated <memory>,
GC> produces the proposed further patch below; could you please
GC> confirm that I can apply it on top of your changes without
GC> offending clang?
GC> 
GC> ---->8---->8---->8----
GC> diff --git a/ce_product_name.cpp b/ce_product_name.cpp
GC> index 019a3782..6e32b4ab 100644
GC> --- a/ce_product_name.cpp
GC> +++ b/ce_product_name.cpp
GC> @@ -37,6 +37,7 @@
GC>  
GC>  #include <algorithm>                    // find()
GC>  #include <istream>
GC> +#include <ostream>
GC>  
GC>  namespace
GC>  {

 This part definitely won't create any problem.

GC> diff --git a/ce_product_name.hpp b/ce_product_name.hpp
GC> index a509baa1..50887cc7 100644
GC> --- a/ce_product_name.hpp
GC> +++ b/ce_product_name.hpp
GC> @@ -26,6 +26,7 @@
GC>  
GC>  #include "mc_enum.hpp"
GC>  
GC> +#include <iosfwd>
GC>  #include <string>
GC>  #include <vector>
GC>  
GC> diff --git a/rate_table.cpp b/rate_table.cpp
GC> index 580c3974..66a0a73d 100644
GC> --- a/rate_table.cpp
GC> +++ b/rate_table.cpp
GC> @@ -43,7 +43,6 @@
GC>  #include <istream>
GC>  #include <limits>
GC>  #include <map>
GC> -#include <memory>
GC>  #include <optional>
GC>  #include <ostream>
GC>  #include <sstream>
GC> diff --git a/rate_table.hpp b/rate_table.hpp
GC> index a90d18f4..914d0648 100644
GC> --- a/rate_table.hpp
GC> +++ b/rate_table.hpp
GC> @@ -30,7 +30,6 @@
GC>  #include <cstdint>
GC>  #include <iosfwd>
GC>  #include <memory>                       // shared_ptr
GC> -#include <ostream>
GC>  #include <string>
GC>  #include <vector>

 But this one would result in errors similar to the ones that made me make
this change initially:

---------------------------------- >8 --------------------------------------
% make rate_table.o
  CXX      rate_table.o
In file included from rate_table.cpp:24:
rate_table.hpp:191:8: error: invalid operands to binary expression 
('std::ostream' (aka 'basic_ostream<char>') and 'int')
    os << number.value();
    ~~ ^  ~~~~~~~~~~~~~~
/usr/lib/llvm-10/bin/../include/c++/v1/type_traits:4050:3: note: candidate 
function template not viable: cannot convert argument of incomplete type 
'std::ostream' (aka 'basic_ostream<char>') to 'std::byte' for 1st argument
---------------------------------- >8 --------------------------------------
and more lines of similar output.

 OTOH inclusion of <ostream> could definitely be removed from
rate_table.cpp, as it's now included from rate_table.hpp.

GC> > - https://github.com/vadz/lmi/pull/140 Stop using std::bind{1st,2nd}()
GC> >   deprecated in C++17: this is the biggest change, although I still tried
GC> >   to keep it as small as possible.
GC> 
GC> Thanks--I can hardly read this code at all, with "bind[12]" or with lambdas
GC> (whenever I want to understand it, I read only the "ET !!" comments),
GC> but I certainly agree that the "bind[12]" stuff must be eradicated.
GC> 
GC> Even after pulling this PR, this command:
GC>   grep 'std::bind[12]' *.?pp
GC> finds more occurrences. Do you want to lambda-ize those, too?

 I see only 3 other occurrences and 2 of them are inside commented out code
in interest_rates.cpp which I did notice, but decided not to modify because
I don't think this would be especially useful (just removing the commented
out code entirely would arguably be more so), but please let me know if you
feel differently about this.

 The remaining case, in expression_template_0_test.cpp, went unnoticed
because I didn't compile this test with clang, but I should indeed fix it
-- and I'll do it once I have some sleep and can be sure I don't do
anything stupid. Thanks for noticing it!

 And thanks once again for applying these patches!
VZ

pgp6kaTfhJAHd.pgp
Description: PGP signature