[libcxx-commits] [PATCH] D122536: [libc++] Optimize `exception_ptr`, especially for the empty case

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/exception:150
+// remain available in the libc++ library, in addition to being defined inline
+// here in this header. To this end, we use the following macro: It is defined
+// as empty when building the libc++ library, forcing the definitions of the
----------------
I think you mean the `__gnu_inline__` attribute here.


================
Comment at: libcxx/include/exception:153
+// aforementioned functions to be emitted and included in the library. When
+// users of libc++ (re)compile their code, however, the following macro won't
+// be defined, and the __gnu_inline__ attribute will force the definitions to
----------------
Do you mean `_LIBCPP_EXCEPTION_PTR_GNU_INLINE` here? If yes, please just put it here. It's a bit confusing otherwise.


================
Comment at: libcxx/include/exception:154-155
+// users of libc++ (re)compile their code, however, the following macro won't
+// be defined, and the __gnu_inline__ attribute will force the definitions to
+// be inlined.
+#ifndef _LIBCPP_EXCEPTION_PTR_GNU_INLINE
----------------
`__gnu_inline__` doesn't force the functions to be inlined. It disables the generation of the functions.


================
Comment at: libcxx/include/exception:158
+// Make sure the __gnu_inline__ attribute is supported
+#if !defined(__GNUC_GNU_INLINE__) && !defined(__GNUC_STDC_INLINE__)
+#error "this compiler does not appear to support the __gnu_inline__ attribute!"
----------------
You can use `__has_attribute(__gnu_inline__)` to check if the compiler supports it. That also works for any other attribute.


================
Comment at: libcxx/include/exception:240
+
+_LIBCPP_DIAGNOSTIC_POP // pops CLANG_DIAGNOSTIC_IGNORED("-Wgnu-inline-cpp-without-extern")
+
----------------
I don't think that the comment is necessary here, but I also don't object.


================
Comment at: libcxx/include/exception:242-244
+inline _LIBCPP_INLINE_VISIBILITY void
+swap(exception_ptr& __lhs, exception_ptr& __rhs) _NOEXCEPT
+{ __lhs.swap(__rhs); }
----------------



================
Comment at: libcxx/src/exception.cpp:9-11
+#if !defined(_LIBCPP_ABI_MICROSOFT) && !defined(__GLIBCXX__)
+#define _LIBCPP_EXCEPTION_PTR_GNU_INLINE /*empty*/
+#endif
----------------
Why do you guard the macro definition here? Could you add a short comment here why you are defining this?


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

https://reviews.llvm.org/D122536