Page MenuHomePhabricator

[mlir] Fix ambiguity when building with Clang 14.0.6
ClosedPublic

Authored by aganea on Sep 19 2022, 1:09 PM.

Details

Summary

When building with clang-cl 14.0.6, the previous "hack" to select the overload, does not seem to be enough:

C:\git\llvm-project>ninja check-mlir -C stage1
ninja: Entering directory `stage1'
[1/3] Building CXX object tools\mlir\unittests\Analysis\Pr...iles\MLIRPresburgerTests.dir\IntegerPolyhedronTest.cpp.obj
FAILED: tools/mlir/unittests/Analysis/Presburger/CMakeFiles/MLIRPresburgerTests.dir/IntegerPolyhedronTest.cpp.obj
C:\PROGRA~1\LLVM\bin\clang-cl.EXE  /nologo -TP -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=1 -DMLIR_INCLUDE_TESTS -DMLIR_ROCM_CONVERSIONS_ENABLED=1 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -IC:\git\llvm-project\stage1\tools\mlir\unittests\Analysis\Presburger -IC:\git\llvm-project\mlir\unittests\Analysis\Presburger -IC:\git\llvm-project\stage1\include -IC:\git\llvm-project\llvm\include -IC:\git\llvm-project\mlir\include -IC:\git\llvm-project\stage1\tools\mlir\include -IC:\git\llvm-project\llvm\utils\unittest\googletest\include -IC:\git\llvm-project\llvm\utils\unittest\googlemock\include /GS- /D_ITERATOR_DEBUG_LEVEL=0 /arch:AVX /Zc:inline /Zc:__cplusplus /Zi -gcodeview-ghash /Oi /Brepro /bigobj /permissive- /W4  -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported /Gw -Werror=mismatched-tags /MT /O2 /Ob2  -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments /EHs-c- /GR- -UNDEBUG -Wno-suggest-override -std:c++17 /showIncludes /Fotools\mlir\unittests\Analysis\Presburger\CMakeFiles\MLIRPresburgerTests.dir\IntegerPolyhedronTest.cpp.obj /Fdtools\mlir\unittests\Analysis\Presburger\CMakeFiles\MLIRPresburgerTests.dir\ -c -- C:\git\llvm-project\mlir\unittests\Analysis\Presburger\IntegerPolyhedronTest.cpp
C:\git\llvm-project\mlir\unittests\Analysis\Presburger\IntegerPolyhedronTest.cpp(1484,21): error: call to member function 'containsPointNoLocal' is ambiguous
  EXPECT_TRUE(poly3.containsPointNoLocal({-0, 0}));
              ~~~~~~^~~~~~~~~~~~~~~~~~~~
C:\git\llvm-project\llvm\utils\unittest\googletest\include\gtest/gtest.h(1968,23): note: expanded from macro 'EXPECT_TRUE'
  GTEST_TEST_BOOLEAN_(condition, #condition, false, true, \
                      ^~~~~~~~~
C:\git\llvm-project\llvm\utils\unittest\googletest\include\gtest/internal/gtest-internal.h(1329,34): note: expanded from macro 'GTEST_TEST_BOOLEAN_'
      ::testing::AssertionResult(expression)) \
                                 ^~~~~~~~~~
C:\git\llvm-project\mlir\include\mlir/Analysis/Presburger/IntegerRelation.h(380,3): note: candidate function
  containsPointNoLocal(ArrayRef<MPInt> point) const;
  ^
C:\git\llvm-project\mlir\include\mlir/Analysis/Presburger/IntegerRelation.h(382,3): note: candidate function
  containsPointNoLocal(ArrayRef<int64_t> point) const {
  ^
1 error generated.
ninja: build stopped: subcommand failed.

Diff Detail

Event Timeline

aganea created this revision.Sep 19 2022, 1:09 PM
aganea requested review of this revision.Sep 19 2022, 1:09 PM
arjunp accepted this revision.Sep 19 2022, 1:11 PM
This revision is now accepted and ready to land.Sep 19 2022, 1:11 PM
ftynse added a subscriber: ftynse.Sep 19 2022, 11:31 PM

Explicit conversion is almost always better than relying on hacks, especially the clever ones, that may not be obvious to anyone or even the author in retrospect. With the conversion, the comment isn't really necessary while adding the two-line comment to explain the hack defies the purpose of using the hack for brevity.

ftynse accepted this revision.Sep 19 2022, 11:31 PM

Explicit conversion is almost always better than relying on hacks, especially the clever ones, that may not be obvious to anyone or even the author in retrospect. With the conversion, the comment isn't really necessary while adding the two-line comment to explain the hack defies the purpose of using the hack for brevity.

Yes, this was a mistake and shouldn't have made it in. We'll keep it in mind for next time.

This revision was automatically updated to reflect the committed changes.