Page MenuHomePhabricator

Use VersionTuple for parsing versions in Triple. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible.
ClosedPublic

Authored by jamesfarrell on Nov 18 2021, 6:54 AM.

Diff Detail

Event Timeline

jamesfarrell created this revision.Nov 18 2021, 6:54 AM
jamesfarrell requested review of this revision.Nov 18 2021, 6:54 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 18 2021, 6:54 AM
jamesfarrell retitled this revision from Use VersionTuple for parsing versions in Triple. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible. See also https://github.com/android/ndk/issues/1455. to Use VersionTuple for parsing versions in Triple. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible.See also https://github.com/android/ndk/issues/1455..Nov 18 2021, 6:55 AM
jamesfarrell retitled this revision from Use VersionTuple for parsing versions in Triple. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible.See also https://github.com/android/ndk/issues/1455. to Use VersionTuple for parsing versions in Triple. This makes it possible to distinguish between "16" and "16.0" after parsing, which previously was not possible..
jamesfarrell edited the summary of this revision. (Show Details)

Cleanup Triple::getMacOSXVersion.

Remove an include that's no longer needed.

danalbert accepted this revision.Nov 19 2021, 11:35 AM

Nice, that's a lot of code cleaned up! LGTM, but probably should wait for someone from Apple to weigh in. I think the new formatting for those error messages is better for them too but that's not my call :)

This revision is now accepted and ready to land.Nov 19 2021, 11:35 AM
dexonsmith added a subscriber: arphaman.

@arphaman , can you take a look?

This revision now requires review to proceed.Nov 19 2021, 1:15 PM

Nice, that's a lot of code cleaned up! LGTM, but probably should wait for someone from Apple to weigh in. I think the new formatting for those error messages is better for them too but that's not my call :)

Agreed on both counts. While I generally tried to be careful to preserve existing behavior, it seemed to me that "x86_64-apple-macosx10.9" (for example) should have the version reported in error messages as "10.9", not "10.9.0". But definitely need to hear from someone at Apple about that.

jamesfarrell added inline comments.Nov 20 2021, 7:33 AM
clang/test/Sema/attr-availability-android.c
8

Observation for reviewers: This is the message we wanted to fix that was the motivation for this change.

arphaman accepted this revision.Nov 29 2021, 7:37 PM

Sorry for delay, this LGTM. Thanks for doing this cleanup!

This revision is now accepted and ready to land.Nov 29 2021, 7:37 PM

Sorry for delay, this LGTM. Thanks for doing this cleanup!

Thanks, and no worries about the time (I was out last week as well)

This revision was landed with ongoing or failed builds.Nov 30 2021, 7:44 AM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Nov 30 2021, 9:55 AM

FYI I reverted this due to test failures -- one specific assertion failure is mentioned in https://reviews.llvm.org/rG40d5eeac6cd8.

fhahn added a subscriber: fhahn.Nov 30 2021, 9:56 AM

It is also breaking building llvm-project/llvm/unittests/Support/Host.cpp on macOS: https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/25781/consoleFull

FAILED: unittests/Support/CMakeFiles/SupportTests.dir/Host.cpp.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++  -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests/Support -I/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/unittests/Support -Iinclude -I/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/include -I/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include -I/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googlemock/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wstring-conversion -fdiagnostics-color -O3 -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.9    -Wno-variadic-macros -Wno-gnu-zero-variadic-macro-arguments -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT unittests/Support/CMakeFiles/SupportTests.dir/Host.cpp.o -MF unittests/Support/CMakeFiles/SupportTests.dir/Host.cpp.o.d -o unittests/Support/CMakeFiles/SupportTests.dir/Host.cpp.o -c /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/unittests/Support/Host.cpp
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/unittests/Support/Host.cpp:428:48: error: too many arguments to function call, expected single argument 'Version', have 3 arguments
                .getMacOSXVersion(SystemMajor, SystemMinor, SystemMicro),
                ~~~~~~~~~~~~~~~~~              ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2057:48: note: expanded from macro 'ASSERT_EQ'
# define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                               ^~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2041:63: note: expanded from macro 'GTEST_ASSERT_EQ'
  ASSERT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2)
                                                              ^~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest_pred_impl.h:168:36: note: expanded from macro 'ASSERT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
                                   ^~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest_pred_impl.h:149:39: note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                      ^~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/include/llvm/ADT/Triple.h:353:3: note: 'getMacOSXVersion' declared here
  bool getMacOSXVersion(VersionTuple &Version) const;
  ^
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/unittests/Support/Host.cpp:431:52: error: too many arguments to function call, expected single argument 'Version', have 3 arguments
  ASSERT_EQ(HostTriple.getMacOSXVersion(HostMajor, HostMinor, HostMicro), true);
            ~~~~~~~~~~~~~~~~~~~~~~~~~~~            ^~~~~~~~~~~~~~~~~~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2057:48: note: expanded from macro 'ASSERT_EQ'
# define ASSERT_EQ(val1, val2) GTEST_ASSERT_EQ(val1, val2)
                                               ^~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest.h:2041:63: note: expanded from macro 'GTEST_ASSERT_EQ'
  ASSERT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2)
                                                              ^~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest_pred_impl.h:168:36: note: expanded from macro 'ASSERT_PRED_FORMAT2'
  GTEST_PRED_FORMAT2_(pred_format, v1, v2, GTEST_FATAL_FAILURE_)
                                   ^~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest_pred_impl.h:149:39: note: expanded from macro 'GTEST_PRED_FORMAT2_'
  GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2), \
                                      ^~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/utils/unittest/googletest/include/gtest/gtest_pred_impl.h:77:52: note: expanded from macro 'GTEST_ASSERT_'
  if (const ::testing::AssertionResult gtest_ar = (expression)) \
                                                   ^~~~~~~~~~
/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/llvm/include/llvm/ADT/Triple.h:353:3: note: 'getMacOSXVersion' declared here
  bool getMacOSXVersion(VersionTuple &Version) const;
  ^
2 errors generated.
`

FYI I reverted this due to test failures -- one specific assertion failure is mentioned in https://reviews.llvm.org/rG40d5eeac6cd8.

Thanks! I saw the failures and was going to revert, but you beat me to it.