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

[Greg Chicares]

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

All pushed, in the order given.

> - https://github.com/vadz/lmi/pull/137 Always assume two's complement
>   representation: in C++20 two's complement is the only allowed
>   representation for the integer types

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r0.html
|
| Abstract
|
| There is One True Representation for signed integers, and that representation
| is two’s complement.

It's about time.

>   Incidentally, this PR also suppresses a warning
>   which prevented bourn_cast unit test from compiling with clang.

I thought clang had some way of recognizing gcc pragmata (such as had already
been written in that file). Indeed:

https://clang.llvm.org/docs/UsersManual.html
|
| The following example code will tell Clang or GCC to ignore the -Wall 
warnings:
|
| #pragma GCC diagnostic ignored "-Wall"

Is it really necessary to suppress the gcc and clang warnings separately
in this case?

> - https://github.com/vadz/lmi/pull/139 Miscellaneous clang 10 fixes

Here:
  commit cc1abb51b  Add missing stream headers
can you help me see why the former code below was not strictly conforming?

  grep '[io]stream' rate_table.hpp
      database(std::istream& index_is, std::shared_ptr<std::istream> data_is);
      void save(std::ostream& index_os, std::ostream& data_os);
  inline std::ostream& operator<<(std::ostream& os, table::Number const& number)

AFAICT, both std::istream and std::ostream are mentioned either
 - by reference, or
 - in the template parameter of std::shared_ptr
so I thought the ideal fix would be to add
  #include <iosfwd>
to the header, and both of these
  #include <istream>
  #include <ostream>
to the associated TU, viz., 'rate_table.cpp'.

Similarly, for 'ce_product_name.?pp', I should think
+  #include <iosfwd>
for '.hpp', and
+  #include <istream>
+  #include <ostream>
for '.cpp' would be ideal.

Combining those points, and also removing a repeated <memory>,
produces the proposed further patch below; could you please
confirm that I can apply it on top of your changes without
offending clang?

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

> - https://github.com/vadz/lmi/pull/140 Stop using std::bind{1st,2nd}()
>   deprecated in C++17: this is the biggest change, although I still tried
>   to keep it as small as possible.

Thanks--I can hardly read this code at all, with "bind[12]" or with lambdas
(whenever I want to understand it, I read only the "ET !!" comments),
but I certainly agree that the "bind[12]" stuff must be eradicated.

Even after pulling this PR, this command:
  grep 'std::bind[12]' *.?pp
finds more occurrences. Do you want to lambda-ize those, too?

>   Several further possible
>   improvements to this code are possible

Ideally, I'd write an expression-template implementation of APL (or at
least a significant subset thereof). But maybe <valarray> would get us
most of the way there.