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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM. I might make a minor pass on the test to make it look more like our other tests (in particular try to avoid duplication of constexpr and non-constexpr tests), but overall this LGTM.

Thanks a whole lot for working on this, we really needed someone to step up and implement this with the help of the compiler.



================
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)
----------------
jyknight wrote:
> 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.
Yes, I am sure the bootstrapping build is running with the just-built Clang. Is it possible that CI ran alongside with the parent commit too? I think that's how it works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120634