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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone 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.
IMHO `#include <cstdint>` is fine (but please move it up with the others and alphabetize).
(If @ldionne disagrees, go with what he says.)


================
Comment at: libcxx/include/source_location:43-44
+class source_location {
+  // Note: the name of the struct '__impl', and the names of its fields, are
+  // REQUIRED by __builtin_source_location, and cannot be changed.
+  struct __impl {
----------------
philnik wrote:
> 
Nit: `s/REQUIRED/required/` (the emphasis is unnecessary, and potentially briefly confusing in that `REQUIRED` has a special meaning in tests). I might reword this comment as
```
// The names source_location::__impl, _M_file_name, _M_function_name, _M_line, and _M_column
// are hard-coded in the compiler and must not be changed here.
```
(Note for users of `git grep`: These names are not hard-coded in Clang //yet//; they're part of D120159 which has not yet landed.)


================
Comment at: libcxx/include/source_location:57-58
+  // in the context of the caller. An explicit value should never be provided.
+  static consteval source_location current(
+      decltype(__builtin_source_location()) __ptr = __builtin_source_location()) noexcept {
+    source_location __sl;
----------------
My kneejerk reaction here was "this indentation is messed up," but then my eyes refocused. ;)
I suggest reflowing as
```
static consteval
source_location current(const void *__ptr = __builtin_source_location()) noexcept {
```
Using `const void *` instead of `decltype(__builtin_source_location())` helps readability and doesn't seem to hurt anything since you're already doing the cast on line 61. In fact, at that point I'd remove the comment on line 60 because now the cast isn't unusual-looking at all.

(2) Should we use `_LIBCPP_HIDE_FROM_ABI` here, even though it's consteval? I don't know.


================
Comment at: libcxx/utils/generate_feature_test_macro_components.py:636-641
   }, {
     "name": "__cpp_lib_source_location",
     "values": { "c++20": 201907 },
     "headers": ["source_location"],
-    "unimplemented": True,
+    "test_suite_guard": "__has_builtin(__builtin_source_location)",
+    "libcxx_guard": "__has_builtin(__builtin_source_location)",
----------------
Please `git grep source_location ../libcxx/docs/` and see whether you can check off any relevant papers or issues. (Also of course `git grep source_location ../libcxx` in general.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120634