[C++] FieldRef with clang-cl from VS 17.4.3 needs operator== with /std:c++20 #15098

lukester1975 · 2022-12-27T18:10:41Z

Describe the bug, including details regarding any error messages, version, and platform.

Hi

Compiling

#include <arrow/api.h>

with clang-cl test.cpp -I/path/to/arrow /std:c++20

fails:

C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\xutility(4588,14): error:
      no matching function for call to object of type 'std::equal_to<>'
        if (!_Pred(*_UFirst1, *_UFirst2)) {
             ^~~~~
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\xutility(4605,17): note:
      in instantiation of function template specialization 'std::equal<const arrow::FieldRef
      *, const arrow::FieldRef *, std::equal_to<>>' requested here
    return _STD equal(_First1, _Last1, _First2, equal_to<>{});
                ^
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\vector(2293,21): note:
      in instantiation of function template specialization 'std::equal<const arrow::FieldRef
      *, const arrow::FieldRef *>' requested here
        return _STD equal(_Left._Unchecked_begin(), _Left._Unchecked_end(), _Right._U...
                    ^
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\xstddef(213,43): note:
      in instantiation of function template specialization 'std::operator==<arrow::FieldRef,
      std::allocator<arrow::FieldRef>>' requested here
        return static_cast<_Ty1&&>(_Left) == static_cast<_Ty2&&>(_Right);
                                          ^
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\variant(1326,20): note:
      in instantiation of function template specialization 'std::equal_to<>::operator()<const

      std::vector<arrow::FieldRef> &, const std::vector<arrow::FieldRef> &>' requested here
            return _Op{}(_Variant_raw_get<_Idx>(_Left), _Right._Val);
                   ^
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\variant(669,23): note:
      in instantiation of function template specialization
      'std::_Variant_relop_visitor2<std::equal_to<>, bool, arrow::FieldPath,
      std::basic_string<char>,
      std::vector<arrow::FieldRef>>::operator()<std::vector<arrow::FieldRef>, 2ULL>'
      requested here
        _STL_STAMP(4, _STL_VISIT_STAMP);
                      ^
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\variant(714,44): note:
      in instantiation of function template specialization
      'std::_Variant_raw_visit1<1>::_Visit<std::_Variant_relop_visitor2<std::equal_to<>,
      bool, arrow::FieldPath, std::basic_string<char>, std::vector<arrow::FieldRef>>, const
      std::_Variant_storage_<false, arrow::FieldPath, std::basic_string<char>,
      std::vector<arrow::FieldRef>> &>' requested here
    return _Variant_raw_visit1<_Strategy>::_Visit(_Idx, static_cast<_Fn&&>(_Func), st...
                                           ^
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\variant(1340,12): note:
      in instantiation of function template specialization 'std::_Variant_raw_visit<const
      std::_Variant_storage_<false, arrow::FieldPath, std::basic_string<char>,
      std::vector<arrow::FieldRef>> &, std::_Variant_relop_visitor2<std::equal_to<>, bool,
      arrow::FieldPath, std::basic_string<char>, std::vector<arrow::FieldRef>>>' requested
      here
        && _Variant_raw_visit(_Right_index, _Right._Storage(), _Visitor{_Left._Storage()});
           ^
cpp/src\arrow/type.h(1729,59): note: in instantiation of function template specialization
      'std::operator==<arrow::FieldPath, std::basic_string<char>,
      std::vector<arrow::FieldRef>>' requested here
  bool Equals(const FieldRef& other) const { return impl_ == other.impl_; }
                                                          ^
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.34.31933\include\xstddef(210,31): note:
      candidate template ignored: substitution failure [with _Ty1 = const arrow::FieldRef &,
      _Ty2 = const arrow::FieldRef &]: ISO C++20 considers use of overloaded operator '=='
      (with operand types 'const arrow::FieldRef' and 'const arrow::FieldRef') to be
      ambiguous despite there being a unique best viable function
    _NODISCARD constexpr auto operator()(_Ty1&& _Left, _Ty2&& _Right) const

I don't recall seeing this previously, so maybe a change with the 17.4(.3?) release. variant impl, maybe.

A simple bool operator==(const FieldRef& other) const { return Equals(other); } added to FieldRef fixes the compilation. I can made a PR if you like.

Thanks

Component(s)

C++

The text was updated successfully, but these errors were encountered:

westonpace · 2023-01-21T00:41:49Z

It would seem we need a new CI environment for visual studio & C++20. It should be a fairly straightforward addition to https://github.com/apache/arrow/blob/master/.github/workflows/cpp.yml (we already have Windows / C++17). Would you be interested in contributing that?

lukester1975 · 2023-01-21T11:03:02Z

Hmm, I just spent an hour or so trying and this is as close as I got: https://github.com/lukester1975/arrow/actions/runs/3974458277/jobs/6813736037#step:11:1039. This uses windows-vs2022 and tries to convince cmake to use clang-cl.

However, it fails with what looks like clang-cl related issues with building boost.build (Unknown toolset: vcunk)? I think I'll have to tap out at this point, cmake makes me sad...

…tion with clang-cl.

lukester1975 · 2023-01-30T10:33:41Z

Spent a little more time on this. There are various problems with dependencies with a simple addition of a clang-cl environment:

The boost Unknown toolset: vcunk issue is simply that boost 1.75.0 predates VS2022 so doesn't know about the vc143 toolset.
Whilst BOOST_SOURCE_URL can be set from the environment's ARROW_BOOST_URL, it just fails sha validation after download since ARROW_BOOST_BUILD_SHA256_CHECKSUM is (only?) read from versions.txt.
Not sure if that's an oversight or there's some other way to specify a boost version (there's a lot going on there...) but I modded versions.txt to use boost 1.79.0 with the appropriate sha and it that works.
Next up is a weird protobuf resource file issue: https://github.com/lukester1975/arrow/actions/runs/4038346404/jobs/6942758428#step:11:1088, so added a patch to only set /bigobj with cl, not clang-cl (it's implicit there anyway).
Next is snappy just not being aware of clang-cl: Allow compiling with MSVC-compatible compilers like clang-cl google/snappy#152, so patch similarly.
Then orc fails to build, so just turned that off.
Thrift then fails with errors about exceptions disabled, even though I can't see any /EH switches: https://github.com/lukester1975/arrow/actions/runs/4042546242/jobs/6950376636#step:11:1106

At that point I'm stopping, not something I can spend more time on. The changes are here: https://github.com/lukester1975/arrow/tree/add-vs2022-clang-cl

I made a branch with the fix for the actual issue I was reporting. All green: https://github.com/lukester1975/arrow/actions?query=branch%3Afix-clang-cl-c%2B%2B20

Maybe you could consider merging it and adding a CI job to simply attempt to compile the trivial main with clang-cl? Sounds like fixing the whole dependency tree to compile with clang-cl is a lot of work...

Thanks

westonpace · 2023-01-30T19:06:32Z

I apologize for implying it should be straightforward 😆 . I'll see if I can make some headway here. I looked at the fix and my main concern at the moment is I don't understand why it should be needed. FieldRef extends EqualityComparable<FieldRef> which should define the == operator (based on the Equals function). I'll see if I can make further headway on your Windows build (we can work around the thrift issue by just disabling parquet for now) to reproduce.

lukester1975 · 2023-01-30T19:35:13Z

I'm sure/hopeful it'll be a small window of versions where it's an issue. Definitely didn't see it before. I did notice the Windows VS2022 environment has clang-cl 15.0.5, whereas I only have 15.0.1 (pro vs. community, but it's [claiming to be] fully up-to-date) so with any luck it's already fixed...

Thanks

westonpace · 2023-01-30T20:10:21Z

I was able to reproduce. It looks like C++20 adds some new capability to the equals operator (e.g. default equality, inequality) but adding these features has caused some new restrictions on the equality operator. Namely, if a == b is legal then b == a must be legal. The way we were adding equality with util::EqualityComparable didn't quite fit those rules but we can make it work.

lukester1975 · 2023-01-30T20:28:18Z

OK. FWIW, all compiles OK with gcc (12), clang (15.0.7) and cl (all in c++20 mode), hence why I thought it was specific to clang-cl.

…33940) If you have an equality operator then C++20 then clang 15 expects it to be symmetric. The util::EqualityComparable operator== was defined as: `bool operator==(util::EqualityComparable<T>, T)` and there is no equivalent: `bool operator==(T, util::EqualityComparable<T>)` This was later determined to be unintentional strictness and the [standard was clarified](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2468r2.html). This error was relaxed in clang 16. Equality only needs to be symmetric if you want the compiler to provide `!=` for you (which we don't rely on). However, it seems like a worthwhile fix, and it doesn't appear that Clang will be backporting the fix to clang 15 (that is my interpretation of the fact that clang lists the defect as clang-16 in this conformance page: https://clang.llvm.org/cxx_status.html). Furthermore, the old definition allowed for `Scalar::operator==(std::shared_ptr<Scalar> other)` (i.e. comparing with a shared_ptr) and it isn't clear to me that should be allowed (it's generally inconsistent with the rest of the code base). This change seems to have undone that and I made no change to restore it (and instead fixed the references to that odd equality operation). BREAKING CHANGE: The functions `bool Scalar::Equals(const std::shared_ptr<Scalar>&) const` and `bool Scalar::Equals(const std::shared_ptr<Scalar>&, const EqualOptions&) const` have been removed. Instead `bool Scalar::Equals(const Scalar&) const` and `bool Scalar::Equals(const Scalar&, const EqualOptions&) const` should be used. BREAKING CHANGE: `FileInfo::ToString` now includes the size and mtime. * Closes: #15098 Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

…g 15 (apache#33940) If you have an equality operator then C++20 then clang 15 expects it to be symmetric. The util::EqualityComparable operator== was defined as: `bool operator==(util::EqualityComparable<T>, T)` and there is no equivalent: `bool operator==(T, util::EqualityComparable<T>)` This was later determined to be unintentional strictness and the [standard was clarified](https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2468r2.html). This error was relaxed in clang 16. Equality only needs to be symmetric if you want the compiler to provide `!=` for you (which we don't rely on). However, it seems like a worthwhile fix, and it doesn't appear that Clang will be backporting the fix to clang 15 (that is my interpretation of the fact that clang lists the defect as clang-16 in this conformance page: https://clang.llvm.org/cxx_status.html). Furthermore, the old definition allowed for `Scalar::operator==(std::shared_ptr<Scalar> other)` (i.e. comparing with a shared_ptr) and it isn't clear to me that should be allowed (it's generally inconsistent with the rest of the code base). This change seems to have undone that and I made no change to restore it (and instead fixed the references to that odd equality operation). BREAKING CHANGE: The functions `bool Scalar::Equals(const std::shared_ptr<Scalar>&) const` and `bool Scalar::Equals(const std::shared_ptr<Scalar>&, const EqualOptions&) const` have been removed. Instead `bool Scalar::Equals(const Scalar&) const` and `bool Scalar::Equals(const Scalar&, const EqualOptions&) const` should be used. BREAKING CHANGE: `FileInfo::ToString` now includes the size and mtime. * Closes: apache#15098 Authored-by: Weston Pace <weston.pace@gmail.com> Signed-off-by: Weston Pace <weston.pace@gmail.com>

lukester1975 added the Type: bug label Dec 27, 2022

rok added the Component: C++ label Jan 8, 2023

kou changed the title ~~FieldRef with clang-cl from VS 17.4.3 needs operator== with /std:c++20~~ [C++] FieldRef with clang-cl from VS 17.4.3 needs operator== with /std:c++20 Jan 21, 2023

lukester1975 added a commit to lukester1975/arrow that referenced this issue Jan 29, 2023

apacheGH-15098: [C++] Add FieldRef::operator==() to fix C++20 compila…

4b15bd2

…tion with clang-cl.

github-actions bot mentioned this issue Jan 30, 2023

GH-15098: [C++] fix util::EqualityComparable to compile on clang 15 #33940

Merged

github-actions bot assigned westonpace Jan 30, 2023

westonpace closed this as completed in #33940 Feb 17, 2023

westonpace added this to the 12.0.0 milestone Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++] FieldRef with clang-cl from VS 17.4.3 needs operator== with /std:c++20 #15098

[C++] FieldRef with clang-cl from VS 17.4.3 needs operator== with /std:c++20 #15098

lukester1975 commented Dec 27, 2022 •

edited

Loading

westonpace commented Jan 21, 2023

lukester1975 commented Jan 21, 2023

lukester1975 commented Jan 30, 2023

westonpace commented Jan 30, 2023

lukester1975 commented Jan 30, 2023

westonpace commented Jan 30, 2023

lukester1975 commented Jan 30, 2023

[C++] FieldRef with clang-cl from VS 17.4.3 needs operator== with /std:c++20 #15098

[C++] FieldRef with clang-cl from VS 17.4.3 needs operator== with /std:c++20 #15098

Comments

lukester1975 commented Dec 27, 2022 • edited Loading

Describe the bug, including details regarding any error messages, version, and platform.

Component(s)

westonpace commented Jan 21, 2023

lukester1975 commented Jan 21, 2023

lukester1975 commented Jan 30, 2023

westonpace commented Jan 30, 2023

lukester1975 commented Jan 30, 2023

westonpace commented Jan 30, 2023

lukester1975 commented Jan 30, 2023

lukester1975 commented Dec 27, 2022 •

edited

Loading