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

Tue Mar 1 10:29:29 PST 2022

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Does this mean we'll only implement `source_location` with Clang? Or does GCC provide a similar builtin? If so, are both compatible?

================
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.
We've had numerous discussions about clang-format in the past several months. Please consult the Discord archive for details, but long story short, the current state of things is that we're going to first agree on a `clang-format` configuration that is suitable, and then slowly move towards it for new code. It hasn't happened yet.

Regarding `decltype(__builtin_source_location())`, you could also just use `auto*` instead.

================
Comment at: libcxx/include/source_location:30-31
+
+// TODO: this include is for uint_least32_t; should we just use
+// __UINT_LEAST32_TYPE__ instead of including the header?
+#include <cstdint>
----------------
philnik wrote:
> Quuxplusone wrote:
> > jyknight wrote:
> > > I'd like a reviewer to opine on this question here.
> > IMHO `#include <cstdint>` is fine (but please move it up with the others and alphabetize).
> > (If @ldionne disagrees, go with what he says.)
> Including the header should be fine.
Yeah, I'd rather use `uint_least32_t` and include the header than use the more arcane `__UINT_LEAST32_TYPE__`.

================
Comment at: libcxx/test/std/language.support/support.srcloc/general.pass.cpp:32
+// A default-constructed value.
+constexpr std::source_location empty;
+static_assert(empty.line() == 0);
----------------
I'd like us to test the return type of all these functions. We usually do something like `std::same_as<uint_least32_t> auto line = empty.line();`.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120634