Page MenuHomePhabricator

[libc++] Fix std::type_info comparison
Needs ReviewPublic

Authored by ldionne on Apr 8 2021, 1:11 PM.


Group Reviewers
Restricted Project

As noted by Richard Smith in, the
current implementation of std::type_info less-than comparison was not an
ordering relation. This commit fixes that.

It's technically a behavior change, however anyone relying on the ordering
as given by the previous implementation was relying on something extremely
brittle, so I think it's safe to fix this.

Diff Detail

Unit TestsFailed

4,450 mslibcxx CI Windows > libc++.std/language_support/support_rtti/
Script: -- : 'RUN: at line 20'; 'C:/Program Files/LLVM/bin\clang++.EXE' C:/ws/w16e2-2/llvm-project/libcxx-ci/libcxx/test/std/ -v --target=x86_64-pc-windows-msvc -nostdinc++ -IC:/ws/w16e2-2/llvm-project/libcxx-ci/build/generic-win/include/c++/v1 '-IC:/ws/w16e2-2/llvm-project/libcxx-ci/build/generic-win\include\c++build' '-IC:/ws/w16e2-2/llvm-project/libcxx-ci/libcxx\test/support' -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -D_LIBCPP_HAS_NO_INT128 -std=c++2a -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-atomic-alignment -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -fmodules-cache-path=C:/ws/w16e2-2/llvm-project/libcxx-ci/build/generic-win/test/std/ -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -c -o C:/ws/w16e2-2/llvm-project/libcxx-ci/build/generic-win/test/std/ -DTU1

Event Timeline

ldionne created this revision.Apr 8 2021, 1:11 PM
ldionne requested review of this revision.Apr 8 2021, 1:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 1:11 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Apr 8 2021, 1:12 PM

This is my best attempt at trying to test this, but TBH I'm not sure how good it is.

rjmccall accepted this revision.Apr 8 2021, 3:54 PM

Patch functionally LGTM. Not sure what you're asking about the test script, but I don't know the libc++ test suite.

Seems the Windows build fails, can you look why the XFAIL doesn't work.


I love this kind of comment, however it's now duplicated here and in the implementation. Would it make sense to move this comment in the implementation so we have only one copy?


Also not to sure how good it is. Can we test whether the order is different in the TUs? Would it help to run the test with different linker orders for TU1 and TU2? e.g. TU1 TU2 MAIN and TU2 TU1 MAIN.


Is there a reason to put this function here instead of in MAIN?