Page MenuHomePhabricator

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

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

Details

Reviewers
rjmccall
Group Reviewers
Restricted Project
Summary

As noted by Richard Smith in https://reviews.llvm.org/D97802#2675067, 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

TimeTest
4,450 mslibcxx CI Windows > libc++.std/language_support/support_rtti/type_info::type_info.order.sh.cpp
Script: -- : 'RUN: at line 20'; 'C:/Program Files/LLVM/bin\clang++.EXE' C:/ws/w16e2-2/llvm-project/libcxx-ci/libcxx/test/std/language.support/support.rtti/type.info/type_info.order.sh.cpp -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/language.support/support.rtti/type.info/Output/type_info.order.sh.cpp.dir/t.tmp/ModuleCache -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -c -o C:/ws/w16e2-2/llvm-project/libcxx-ci/build/generic-win/test/std/language.support/support.rtti/type.info/Output/type_info.order.sh.cpp.dir/t.tmp.tu1.o -DTU1

Event Timeline

ldionne created this revision.Thu, Apr 8, 1:11 PM
ldionne requested review of this revision.Thu, Apr 8, 1:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Apr 8, 1:11 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added inline comments.Thu, Apr 8, 1:12 PM
libcxx/test/std/language.support/support.rtti/type.info/type_info.order.sh.cpp
1

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

rjmccall accepted this revision.Thu, Apr 8, 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.

libcxx/include/typeinfo
269

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?

libcxx/test/std/language.support/support.rtti/type.info/type_info.order.sh.cpp
1

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.

36

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