[libcxx-commits] [PATCH] D111516: [libc++][docs] Mark LWG3274 as complete

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 2 inline comments as done.
jloser added inline comments.


================
Comment at: libcxx/include/span:335
     _LIBCPP_INLINE_VISIBILITY constexpr size_type size_bytes() const noexcept { return _Extent * sizeof(element_type); }
-    _LIBCPP_INLINE_VISIBILITY constexpr bool empty()           const noexcept { return _Extent == 0; }
+    [[nodiscard]] _LIBCPP_INLINE_VISIBILITY constexpr bool empty() const noexcept { return _Extent == 0; }
 
----------------
Mordante wrote:
> Quuxplusone wrote:
> > jloser wrote:
> > > Mordante wrote:
> > > > Since the block was manually aligned it would be nice to keep it that way.
> > > With adding `[[nodiscard]]` it won't be aligned with `size()` and `size_bytes()` member function. So, we can either manually align those, or not align `empty()` member function. I chose the latter. WDYT?
> > Yeah, I think @mordante was asking for
> > ```
> >     _LIBCPP_INLINE_VISIBILITY constexpr size_type size()           const noexcept { return _Extent; }
> >     _LIBCPP_INLINE_VISIBILITY constexpr size_type size_bytes()     const noexcept { return _Extent * sizeof(element_type); }
> >     [[nodiscard]] _LIBCPP_INLINE_VISIBILITY constexpr bool empty() const noexcept { return _Extent == 0; }
> > ```
> > FWIW, I think it's fine either way, and would err on the side of the smaller diff (as you did) myself, but I don't care.
> Yes that was indeed what I was asking for. It's just a bit odd; on one hand we spend time to make code looking nicely in the original commit, but one the other hand we want to keep our diffs minimal. But it was a suggestion so no blocker.
I just aligned the neighboring member functions. I don't feel strongly either way. Like Arthur said, I like to keep the diff as small as possible, but I hear your argument around making the code look nice.

In any case, CI passed earlier before the whitespace alignment (https://buildkite.com/llvm-project/libcxx-ci/builds/5884). I'll land this later tonight.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111516