Page MenuHomePhabricator

[libc++] Simplify a few macros in __config

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


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

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

#ifndef MACRO
#  define MACRO ...

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
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.

@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).


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


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.

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=.)

phosek added inline comments.Jun 4 2021, 11:32 AM

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

Quuxplusone added inline comments.Jun 4 2021, 11:36 AM

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.

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.


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.