This is an archive of the discontinued LLVM Phabricator instance.

Provide storage for `true_type::value` and `false_type::value`.
ClosedPublic

Authored by delcypher on Dec 21 2018, 4:07 PM.

Details

Summary

This fixes linker errors that occurs when the
sanitizer_type_traits_test.cc is built without optimizations.

The error occurs because the test tries to take a reference.
A possible workaround is to give the GTest macros take boolean rvalues
by doing something like:

ASSERT_TRUE(bool(is_same<uptr, uptr>::value));

However this only hides the problem. Unfortunately Using constexpr
won't fix the problem unless we are using C++17.

Event Timeline

delcypher created this revision.Dec 21 2018, 4:07 PM
Herald added subscribers: Restricted Project, mgorny. · View Herald TranscriptDec 21 2018, 4:07 PM
vitalybuka added inline comments.Jan 8 2019, 4:29 PM
lib/sanitizer_common/sanitizer_type_traits.cc
17

I guess we can avoid cc file if you use constexpr in the header file.

delcypher added inline comments.Jan 9 2019, 1:09 AM
lib/sanitizer_common/sanitizer_type_traits.cc
17

@vitalybuka I tried that. Unfortunately I still get link errors.

See https://stackoverflow.com/questions/8452952/c-linker-error-with-class-static-constexpr .

An alternative to this patch to just change the test that causes the problem to something like

ASSERT_TRUE(bool(is_same<uptr, uptr>::value));
`

This avoids taking a reference to is_same<T,U>::value and instead constructs a temporary value from it. The ASSERT_TRUE macro then takes a reference to it.

I'm not sure which is better. Changing the test just hides the issue (it will occur again if anyone tries to take a reference to is_same<T,U>::value). However this patch bloats the code for the purpose of a single test, which isn't great either.

Let me know, which you'd prefer.

vitalybuka accepted this revision.Jan 9 2019, 12:47 PM

LGTM as is

lib/sanitizer_common/sanitizer_type_traits.cc
19

please add empty lines to separate namemespace {}

This revision is now accepted and ready to land.Jan 9 2019, 12:47 PM

Adding spacing above and below declarations in namespace.

delcypher marked an inline comment as done.Jan 11 2019, 10:02 AM
delcypher marked 2 inline comments as done.Jan 11 2019, 10:03 AM
This revision was automatically updated to reflect the committed changes.