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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
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;
----------------
ldionne wrote:
> EricWF wrote:
> > philnik wrote:
> > > Quuxplusone wrote:
> > > > 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.
> > > > (2) Should we use _LIBCPP_HIDE_FROM_ABI here, even though it's consteval? I don't know.
> > > 
> > > I don't think so. A `consteval` function can never be part of any ABI, so there is no need to mark it as `_LIBCPP_HIDE_FOM_ABI`.
> > I don't think using `const void*` improves readability. Why do you say that?
> > 
> > @Quuxplusone Going forward the project should take a "format and forget about it" approach to whitespace changes. What's correct is what clang-format spits out. I don't believe reviewing whitespace changes is a good use of engineering effort.
> We've had numerous discussions about clang-format in the past several months. Please consult the Discord archive for details, but long story short, the current state of things is that we're going to first agree on a `clang-format` configuration that is suitable, and then slowly move towards it for new code. It hasn't happened yet.
> 
> Regarding `decltype(__builtin_source_location())`, you could also just use `auto*` instead.
Fantastic. Let's do that. Because reviewing whitespace is in nobodys best interest.
And if we can't agree on a `clang-format` configuration, then we can't agree on whitespace, and we shouldn't bog any review down changing it. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120634