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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
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>
----------------
jyknight wrote:
> I'd like a reviewer to opine on this question here.
Including the header should be fine.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120634