Implements source_location.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
libcxx/include/source_location | ||
---|---|---|
48 | I think we need to figure out this issue before we move forward. | |
libcxx/test/std/language.support/reflection.src_loc/creation/source_location.pass.cpp | ||
29 ↗ | (On Diff #265493) | 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. |
libcxx/test/std/language.support/reflection.src_loc/fields/column.pass.cpp | ||
21 ↗ | (On Diff #265493) | Should we add ASSERT_NOEXCEPT(loc.column())? This also applies to the other accessors AFAICT. |
libcxx/test/std/language.support/reflection.src_loc/fields/file_name.pass.cpp | ||
25 ↗ | (On Diff #265493) | Like mentioned above, this needs to be a libc++ specific assertion. |
libcxx/test/std/language.support/reflection.src_loc/fields/line.pass.cpp | ||
37 ↗ | (On Diff #265493) | Inconsistent indentation. |
Thanks for the review.
libcxx/test/std/language.support/reflection.src_loc/fields/line.pass.cpp | ||
---|---|---|
37 ↗ | (On Diff #265493) | I think that you mistaken it with the above line which parameter list, hence indented differently. |
libcxx/test/std/language.support/reflection.src_loc/fields/line.pass.cpp | ||
---|---|---|
37 ↗ | (On Diff #265493) | Agreed, please disregard that comment. |
libcxx/include/source_location | ||
---|---|---|
48 | I still don't know why it doesn't work with consteval, but I've seen that libstdc++ experimental implementation also uses constexpr here: https://github.com/gitGNU/gcc/blob/master/libstdc%2B%2B-v3/include/experimental/source_location#L50 |
Ok, so clang 8 doesn't support consteval. It also doesn't have required builtins (__builtin_FILE etc.). consteval works ok on clang 9+.
How can I mark tests as unsupported when the builtins are not supported? (a good example would be column.pass.cpp test that I disabled for gcc that doesn't have __builtin_COLUMN)
libcxx/test/std/language.support/support.srcloc/obs/column.pass.cpp | ||
---|---|---|
11 | Should I use XFAIL instead of UNSUPPORTED so that the test breaks when gcc adds this builtin? |
Can you mark it as unsupported on the specific compiler versions?
libcxx/test/std/language.support/support.srcloc/obs/column.pass.cpp | ||
---|---|---|
11 | If you use UNSUPPORTED: gcc-x, gcc-y, gcc-z, then you won't have this problem. WDYT? Otherwise, XFAIL seems to me like a good idea indeed. |
Are you thinking of a single pointer to information stored in the data segment by the compiler?
Yes, sorry. I should have stated that initially. We need source location to be as low cost as possible to copy.
It should only consume a single register.
Please feel free to reach out via email or chat and we can discuss ideas on achieving this in more depth.
I've updated the "unsupported" comments with precise compiler versions. I cannot test apple-clang though.
Anyway, I'll put this revision into hibernation until there's a builtin for a single-pointer source location (whether it be __bultin_source_location or T* __builtin_static_storage_duration(T)).
libcxx/test/libcxx/language.support/support.srcloc/source_location.pass.cpp | ||
---|---|---|
23 | Note to myself: add a test verifying assert(sizeof(source_location) == sizeof(void*)). |
Any updates here? I saw some internal interest around source_location today and was curious about the status of this.
I think we need to figure out this issue before we move forward.