This is an archive of the discontinued LLVM Phabricator instance.

[libc++] [P1208] [C++20] Adopt source location from Library Fundamentals V3 for C++20.
AbandonedPublic

Authored by curdeius on May 21 2020, 6:30 AM.

Details

Reviewers
ldionne
EricWF
Group Reviewers
Restricted Project
Summary

Implements source_location.

Diff Detail

Event Timeline

curdeius created this revision.May 21 2020, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 21 2020, 6:30 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius updated this revision to Diff 265493.May 21 2020, 6:39 AM
  • Add new header to CMake, double-include test and module map.
ldionne requested changes to this revision.May 21 2020, 12:13 PM

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
30

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
22

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
26

Like mentioned above, this needs to be a libc++ specific assertion.

libcxx/test/std/language.support/reflection.src_loc/fields/line.pass.cpp
38

Inconsistent indentation.

This revision now requires changes to proceed.May 21 2020, 12:13 PM
curdeius planned changes to this revision.May 22 2020, 12:22 AM
curdeius marked 2 inline comments as done.

Thanks for the review.

libcxx/test/std/language.support/reflection.src_loc/fields/line.pass.cpp
38

I think that you mistaken it with the above line which parameter list, hence indented differently.

ldionne added inline comments.May 25 2020, 11:06 AM
libcxx/test/std/language.support/reflection.src_loc/fields/line.pass.cpp
38

Agreed, please disregard that comment.

curdeius updated this revision to Diff 271644.Jun 18 2020, 3:25 AM
curdeius marked an inline comment as done.
  • Rebase.
  • Address review comments.
curdeius marked an inline comment as done.Jun 18 2020, 3:31 AM
curdeius added inline comments.
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
Also, I've just rechecked it and it only fails with gcc, it passes all the tests with clang 10.
I remember however doing some manual testing on clang on Windows (an older version, probably 9) and it was failing too.
I'll check how other clang versions behave.

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)

curdeius marked an inline comment as done.Jun 18 2020, 4:57 AM
curdeius added inline comments.
libcxx/test/std/language.support/support.srcloc/obs/column.pass.cpp
10 ↗(On Diff #271644)

Should I use XFAIL instead of UNSUPPORTED so that the test breaks when gcc adds this builtin?

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)

Can you mark it as unsupported on the specific compiler versions?

libcxx/test/std/language.support/support.srcloc/obs/column.pass.cpp
10 ↗(On Diff #271644)

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.

EricWF requested changes to this revision.Jun 18 2020, 11:05 AM

This is not the ABI we want.

This revision now requires changes to proceed.Jun 18 2020, 11:05 AM

This is not the ABI we want.

Are you thinking of a single pointer to information stored in the data segment by the compiler?

This is not the ABI we want.

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.

curdeius updated this revision to Diff 271943.Jun 19 2020, 12:58 AM
  • Use precise compiler versions in UNSUPPORTED.
curdeius planned changes to this revision.Jun 19 2020, 1:02 AM
curdeius marked an inline comment as done.

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

curdeius added inline comments.Jun 19 2020, 1:04 AM
libcxx/test/libcxx/language.support/support.srcloc/source_location.pass.cpp
23 ↗(On Diff #271943)

Note to myself: add a test verifying assert(sizeof(source_location) == sizeof(void*)).

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

Any updates here? I saw some internal interest around source_location today and was curious about the status of this.

Xavier added a subscriber: Xavier.Nov 9 2021, 12:12 AM