[libcxx-commits] [PATCH] D80376: [libc++] [P1208] [C++20] Adopt source location from Library Fundamentals V3 for C++20.

[Louis Dionne via Phabricator via libcxx-commits]

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

Thanks for the contribution! This looks pretty good, however I would like to see tests where `current()` is called in some full-expression with other expressions in it, in a default member initializer, etc.

http://eel.is/c++draft/support.srcloc.class#lib:source_location specifies the intended behavior of `source_location` in various special cases (like http://eel.is/c++draft/support.srcloc.class#support.srcloc.cons-2), and we should have tests for those.

Also, is there a reason why the runtime and the constexpr tests can't be identical to each other? It looks like some of your coverage is only on constexpr tests, and other coverage is only on the runtime tests.



================
Comment at: libcxx/include/source_location:47
+struct source_location {
+  // TODO: current() should be marked consteval instead of constexpr,
+  // but current compiler implementations seem buggy and return callee's location.
----------------
I think we need to figure out this issue before we move forward.


================
Comment at: libcxx/test/std/language.support/reflection.src_loc/creation/source_location.pass.cpp:29
+  assert(loc.column() == 0u);
+  assert(SV{loc.file_name()} == "unknown");
+  assert(SV{loc.function_name()} == "unknown");
----------------
We can't test it that way because the standard says it's unspecified what it returns. I think those need to be libc++ specific tests.


================
Comment at: libcxx/test/std/language.support/reflection.src_loc/fields/column.pass.cpp:21
+
+bool test() {
+  std::source_location loc1;
----------------
Should we add `ASSERT_NOEXCEPT(loc.column())`? This also applies to the other accessors AFAICT.


================
Comment at: libcxx/test/std/language.support/reflection.src_loc/fields/file_name.pass.cpp:25
+  std::source_location loc1;
+  assert(SV{loc1.file_name()} == "unknown");
+  std::source_location loc2 = std::source_location::current();
----------------
Like mentioned above, this needs to be a libc++ specific assertion.


================
Comment at: libcxx/test/std/language.support/reflection.src_loc/fields/line.pass.cpp:37
+    std::source_location loc = std::source_location::current()) {
+  assert(loc.line() > __LINE__);
+
----------------
Inconsistent indentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80376