dsharlet / array

C++ multidimensional arrays in the spirit of the STL
Apache License 2.0
198 stars 15 forks source link

Add missing `#include <algorithm>` #61

Closed fbleibel-g closed 2 years ago

fbleibel-g commented 2 years ago

An upcoming compiler release updates libc++ to stop exporting <algorithm> from certain headers [1]. This means code using functions from <algorithm> but relying on them existing from transitive header dependencies will now be broken.

[1] The commit: https://github.com/llvm/llvm-project/commit/2e2f3158c604adb8401a2a44a03f58d4b6f1c7f9

dsharlet commented 2 years ago

This looks good especially as a quick fix, but I am wondering which functions we actually use from <algorithm>? If it's only min and max on index_t, we should probably just break that dependency.

fbleibel-g commented 2 years ago

These are all the errors without <algorithm> (-ferror-limit=0):

./third_party/array/array.h:329:15: error: no member named 'min' in namespace 'std'; did you mean 'fmin'?
  return std::min(std::max(x, min), max);
         ~~~~~^~~
              fmin
third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/cmath:421:9: note: 'fmin' declared here
using ::fmin _LIBCPP_USING_IF_EXISTS;
        ^
./third_party/array/array.h:329:24: error: no member named 'max' in namespace 'std'; did you mean 'fmax'?
  return std::min(std::max(x, min), max);
                  ~~~~~^~~
                       fmax
third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/cmath:419:9: note: 'fmax' declared here
using ::fmax _LIBCPP_USING_IF_EXISTS;
        ^
./third_party/array/array.h:491:26: error: no member named 'min' in namespace 'std'
      index_t max = std::min(i.max(), outer_max);
                    ~~~~~^
./third_party/array/array.h:557:37: error: no member named 'min' in namespace 'std'
  return {{interval<>(v.min(), std::min(inner_extent, v.extent())), v.max()},
                               ~~~~~^
./third_party/array/array.h:609:15: error: no member named 'min' in namespace 'std'
  return std::min(first, variadic_min(rest...));
         ~~~~~^
./third_party/array/array.h:615:15: error: no member named 'max' in namespace 'std'; did you mean 'fmax'?
  return std::max(first, variadic_max(rest...));
         ~~~~~^~~
              fmax
third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/cmath:419:9: note: 'fmax' declared here
using ::fmax _LIBCPP_USING_IF_EXISTS;
        ^
./third_party/array/array.h:645:52: error: unexpected type name 'index_t': expected expression
      (std::get<Is>(dims).extent() - 1) * std::min<index_t>(0, std::get<Is>(dims).stride())...);
                                                   ^
./third_party/array/array.h:645:48: error: no member named 'min' in namespace 'std'
      (std::get<Is>(dims).extent() - 1) * std::min<index_t>(0, std::get<Is>(dims).stride())...);
                                          ~~~~~^
./third_party/array/array.h:651:52: error: unexpected type name 'index_t': expected expression
      (std::get<Is>(dims).extent() - 1) * std::max<index_t>(0, std::get<Is>(dims).stride())...);
                                                   ^
./third_party/array/array.h:651:48: error: no member named 'max' in namespace 'std'; did you mean 'fmax'?
      (std::get<Is>(dims).extent() - 1) * std::max<index_t>(0, std::get<Is>(dims).stride())...);
                                          ~~~~~^~~
                                               fmax
third_party/crosstool/v18/llvm_unstable/toolchain/bin/../include/c++/v1/cmath:419:9: note: 'fmax' declared here
using ::fmax _LIBCPP_USING_IF_EXISTS;
        ^
./third_party/array/array.h:796:21: error: unexpected type name 'index_t': expected expression
    return std::max<index_t>(1, abs(dim.stride()) * dim.extent());
                    ^
./third_party/array/array.h:796:17: error: no member named 'max' in namespace 'std'
    return std::max<index_t>(1, abs(dim.stride()) * dim.extent());
           ~~~~~^
./third_party/array/array.h:1315:62: error: unexpected type name 'size_t': expected expression
      std::tuple_cat(inner_dim(), tuple_of_n<dim<>, std::max<size_t>(1, Rank) - 1>()));
                                                             ^
./third_party/array/array.h:1315:58: error: no member named 'max' in namespace 'std'
      std::tuple_cat(inner_dim(), tuple_of_n<dim<>, std::max<size_t>(1, Rank) - 1>()));
                                                    ~~~~~^
./third_party/array/array.h:1315:84: error: expected expression
      std::tuple_cat(inner_dim(), tuple_of_n<dim<>, std::max<size_t>(1, Rank) - 1>()));
                                                                                   ^
./third_party/array/array.h:1379:22: error: no member named 'max' in namespace 'std'
  index_t min = std::max(a.min(), b.min());
                ~~~~~^
./third_party/array/array.h:1380:17: error: no member named 'min' in namespace 'std'; did you mean simply 'min'?
  index_t max = std::min(a.max(), b.max());
                ^~~~~~~~
                min
./third_party/array/array.h:1379:11: note: 'min' declared here
  index_t min = std::max(a.min(), b.min());
          ^
17 errors generated.

What do you think - should we remove std::min and std::max? We depend on other stdlib headers so maybe this isn't so bad.

(side question - should std::min(std::max(x, min), max); (at line 329) be replaced with std::clamp?)

dsharlet commented 2 years ago

I think we should. min and max are trivial one-liners to duplicate. All the other STL headers we include seem like they should be tiny in comparison to <algorithm> (maybe <tuple> is big...?). The LLVM change motivating this was probably made with the same reasoning.

That min-max could be replaced with std::clamp, but if we replace it with our own helpers, I don't think adding a clamp helper makes sense for only one use.