[libcxx-commits] [PATCH] D120634: [Libcxx] Add <source_location> header.

Tue Mar 1 15:25:20 PST 2022

jyknight added inline comments.

================
Comment at: libcxx/include/source_location:57-58
+  // in the context of the caller. An explicit value should never be provided.
+  static consteval source_location current(
+      decltype(__builtin_source_location()) __ptr = __builtin_source_location()) noexcept {
+    source_location __sl;
----------------
EricWF wrote:
> philnik wrote:
> > Quuxplusone wrote:
> > > My kneejerk reaction here was "this indentation is messed up," but then my eyes refocused. ;)
> > > I suggest reflowing as
> > > ```
> > > static consteval
> > > source_location current(const void *__ptr = __builtin_source_location()) noexcept {
> > > ```
> > > Using `const void *` instead of `decltype(__builtin_source_location())` helps readability and doesn't seem to hurt anything since you're already doing the cast on line 61. In fact, at that point I'd remove the comment on line 60 because now the cast isn't unusual-looking at all.
> > > 
> > > (2) Should we use `_LIBCPP_HIDE_FROM_ABI` here, even though it's consteval? I don't know.
> > > (2) Should we use _LIBCPP_HIDE_FROM_ABI here, even though it's consteval? I don't know.
> > 
> > I don't think so. A `consteval` function can never be part of any ABI, so there is no need to mark it as `_LIBCPP_HIDE_FOM_ABI`.
> I don't think using `const void*` improves readability. Why do you say that?
> 
> @Quuxplusone Going forward the project should take a "format and forget about it" approach to whitespace changes. What's correct is what clang-format spits out. I don't believe reviewing whitespace changes is a good use of engineering effort.
Can't use `auto*` in the argument type -- that'd make this into a function template.

I don't want to use a `const void *` argument, because constexprs are not allowed to cast back _from_ `void*`, even if the underlying object is a `T` and you're casting to `T*`. (However, note that I did add a hack to Clang to support libstdc++ -- which DOES have that invalid cast that GCC doesn't diagnose, but I wouldn't want to introduce the same issue into libc++.)

I can move the decltype into a separate `using __bsl_ty = decltype(__builtin_source_location());` line to improve readability.

================
Comment at: libcxx/include/source_location:64
+  }
+  _LIBCPP_HIDE_FROM_ABI constexpr source_location() noexcept = default;
+
----------------
EricWF wrote:
> When does this constructor get called?
> 
> I assume it's in the spec?
Not sure what you mean. Yes, a default constructor is required by the spec.

================
Comment at: libcxx/test/std/language.support/support.srcloc/general.pass.cpp:16-19
+// Only run tests when support is available; it's not yet universally available
+// in c++20 compilers. TODO: would it be better to exclude this test with some
+// more "UNSUPPORTED" annotations?
+#if defined(__cpp_lib_source_location)
----------------
ldionne wrote:
> Mordante wrote:
> > philnik wrote:
> > > Mordante wrote:
> > > > jyknight wrote:
> > > > > philnik wrote:
> > > > > > This should be something along the lines of `// UNSUPPORTED: clang-13`, depending on when `__builtin_source_location was implemented`.
> > > > > Well, it's not implemented until the patch this depends on. So it'll be implemented as of clang-15 -- but current builds of clang-15 don't have that support. Should I just put "// UNSUPPORTED: clang-13, clang-14"? Will that cause problems with test-bots? I'll submit the other patch first -- but do libcxx bots run with older builds of unreleased head clang? Guess I'll just try and see what happens...
> > > > Since the clang part is under review it should be also unsupported in clang-13 and apple-clang-12.
> > > > All our supported platforms are tested in the precommit CI so it's possible to see which other platforms fail.
> > > CI doesn't run a trunk clang, so it should just be `// UNSUPPORTED: clang-13 clang-14` and the others that don't work.
> > > Well, it's not implemented until the patch this depends on. So it'll be implemented as of clang-15 -- but current builds of clang-15 don't have that support. Should I just put "// UNSUPPORTED: clang-13, clang-14"? Will that cause problems with test-bots? I'll submit the other patch first -- but do libcxx bots run with older builds of unreleased head clang? Guess I'll just try and see what happens...
> > 
> > The CI uses nightly Ubuntu builds, but the Docker file isn't automatically updated. This means:
> > - we first need to land the clang patch
> > - wait a short while before it's available in Ubuntu
> > - ask @ldionne to update the Docker image
> > - wait a while until the new image is used in the CI
> > This isn't ideal, but we've used this approach for other patches with dependencies on the CI running the latest version of our Docker file.
> @Mordante is correct -- basically we'll need to wait a bit after the Clang patch lands for the Docker images to be refreshed and then all CI will be running with a Clang that supports the builtin. In the meantime, only the Bootstraping build bot would is relevant, since it's the only one that's going to run this test.
The GCC bot should be running the test already.

Are you sure the bootstrapping build is doing this? Because if so, it should've failed (as clang-15 isn't marked unsupported), but it didn't.

================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:636-641
   }, {
     "name": "__cpp_lib_source_location",
     "values": { "c++20": 201907 },
     "headers": ["source_location"],
-    "unimplemented": True,
+    "test_suite_guard": "__has_builtin(__builtin_source_location)",
+    "libcxx_guard": "__has_builtin(__builtin_source_location)",
----------------
Quuxplusone wrote:
> Please `git grep source_location ../libcxx/docs/` and see whether you can check off any relevant papers or issues. (Also of course `git grep source_location ../libcxx` in general.)
Did that already in the preceding update.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120634