This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Simplify a few macros in __config
ClosedPublic

Authored by ldionne on Jun 4 2021, 10:56 AM.

Details

Reviewers
None
Group Reviewers
Restricted Project
Commits
rGfb4e4646188c: [libc++] Simplify a few macros in __config
Summary

Several macros were guarded with a check along the lines of:

#ifndef MACRO
#  define MACRO ...
#endif

However, some of these macros are never intended to be defined by users,
so it's pointless to make this check (i.e. the first #ifndef is always
true). This commit removes those checks.

The motivation for doing this cleanup is to remove the impression that
arbitrary configurations macros can be defined by users when including
libc++ headers, which doesn't work reliably and leads to macro spaghetti.
If one needs to be able to override a knob in the config, that's fine,
but the proper way to do that is to document the macro as being a public
facing knob in the documentation, and most likely to migrate that macro
to
config_site (depending on the nature of the macro).

Diff Detail

Event Timeline

ldionne created this revision.Jun 4 2021, 10:56 AM
ldionne requested review of this revision.Jun 4 2021, 10:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2021, 10:56 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne edited the summary of this revision. (Show Details)Jun 4 2021, 10:56 AM
ldionne added inline comments.
libcxx/include/__config
493

@vitalybuka Just pinging you in case you can think of someone that might be broken by this (i.e. they would have to manually define _LIBCPP_HAS_NO_ASAN on a compiler that pretends that __has_feature(address_sanitizer) is true).

810–811

@EricWF In case you know someone that defines _LIBCPP_PREFERRED_OVERLOAD to something else manually?

1300

This looks like a bug to me, but I'm not sure who added those thread safety annotations or when they are used. @phosek I have a feeling I've seen you touch those at some point?

Quuxplusone added inline comments.
libcxx/include/__config
545–547

I would strongly (but unfortunately with low authority-to-override-Louis) prefer to keep most or all of these #ifs. These #ifs seem to make it much easier to check the codegen with and without a certain feature, e.g. to compare

g++ -D_LIBCPP_HAS_NO_ASAN test.cpp -S
g++ -U_LIBCPP_HAS_NO_ASAN test.cpp -S
g++ -D_LIBCPP_NODEBUG_TYPE= test.cpp -S
g++ -D_LIBCPP_NODEBUG_TYPE='[[clang::nodebug]]' test.cpp -S

on Godbolt or something, instead of having to manually edit and rebuild the libc++ headers locally.

(I've used this same pattern for my p1144 _LIBCPP_TRIVIALLY_RELOCATABLE macro, so that you can use Godbolt to compare the codegen with -D_LIBCPP_TRIVIALLY_RELOCATABLE=[[trivially_relocatable]] against the codegen with -D_LIBCPP_TRIVIALLY_RELOCATABLE=.)
https://p1144.godbolt.org/z/h4Torssz4

phosek added inline comments.Jun 4 2021, 11:32 AM
libcxx/include/__config
1300

I added some thread annotations but this predates me, I agree that this looks like a bug.

libcxx/include/__config
1300

I'll bite: In what way does this look like a bug? Am I missing something? It looks like it's just saying "If _LIBCPP_HAS_THREAD_SAFETY_ANNOTATIONS is defined from line 1294 above (or from the user), then make _LIBCPP_THREAD_SAFETY_ANNOTATION(x) do something (if the user hasn't already made it do something else); otherwise make it do nothing." That sounds to me like a reasonable bit of code.

ldionne accepted this revision as: Restricted Project.Jun 7 2021, 9:45 AM
ldionne marked 3 inline comments as done.
ldionne added inline comments.
libcxx/include/__config
545–547

While I acknowledge that it makes that specific use case harder to do, I think the benefits of clarity and ease of long term evolution (since we know nobody's trying to define these macros in the wild) gives us more bang for the buck.

1300

I must agree with Arthur here - I'm not seeing anymore what I thought was a bug.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 7 2021, 9:46 AM
This revision was automatically updated to reflect the committed changes.
ldionne marked 2 inline comments as done.