[libcxx-commits] [PATCH] D103705: [libc++] Simplify a few macros in __config

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: libc++.
ldionne marked 3 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/include/__config:545-547
+#if !defined(__SANITIZE_ADDRESS__)
 #define _LIBCPP_HAS_NO_ASAN
 #endif
----------------
Quuxplusone wrote:
> I would strongly (but unfortunately with low authority-to-override-Louis) prefer to keep most or all of these `#if`s. These `#if`s 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
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.


================
Comment at: libcxx/include/__config:1300
-#ifndef _LIBCPP_THREAD_SAFETY_ANNOTATION
-#  ifdef _LIBCPP_HAS_THREAD_SAFETY_ANNOTATIONS
-#    define _LIBCPP_THREAD_SAFETY_ANNOTATION(x) __attribute__((x))
----------------
Quuxplusone wrote:
> phosek wrote:
> > ldionne wrote:
> > > 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?
> > I added some thread annotations but this predates me, I agree that this looks like a bug.
> 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.
I must agree with Arthur here - I'm not seeing anymore what I thought was a bug.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103705/new/

https://reviews.llvm.org/D103705