[libcxx-commits] [PATCH] D68365: [libc++] Implement P1004R2 (constexpr std::vector)

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF added inline comments.


================
Comment at: libcxx/include/__bit_reference:358-361
+    if (__libcpp_is_constant_evaluated())
+        std::fill_n(std::__to_address(__first.__seg_), 0, __nw);
+    else
+        _VSTD::memset(_VSTD::__to_address(__first.__seg_), 0, __nw * sizeof(__storage_type));
----------------
var-const wrote:
> ldionne wrote:
> > I think we should always call `std::fill_n` here instead. Indeed, both are equivalent, but `std::fill_n` is nicer for various reasons.
> > 
> > I know `fill_n` doesn't end up calling `memset` directly, however the compiler will end up calling `memset` (when optimized). IIRC, @var-const was actually about to add this optimization to `fill_n` explicitly, when we realized it didn't have an impact on the code generation. @var-const can you confirm?
> > 
> > Also, I think we need to add a comment in `std::fill_n` that explains this. I think it can be done in this patch since we're switching from `memset` to `fill_n`.
> > 
> > This comment applies elsewhere in this patch too.
> I can confirm that working on https://reviews.llvm.org/D118329, I found that the compiler would replace `uninitialized_fill{,_n}` with calls to `memset` in an optimized build, which made it unnecessary to write that optimization explicitly. I would expect `fill_n` to work similarly, but I have only tried with the uninitialized algorithms. FWIW, I have a lingering suspicion that there might be some corner cases (related to aliasing, perhaps) where the compiler would fail to do the optimization, but it was absolutely reliable in all the (simple) cases I checked.
@ldionne @var-const Can we get a godbolt showing a call to fill_n that gets lowered to memset?


================
Comment at: libcxx/include/vector:428
 
-    _LIBCPP_INLINE_VISIBILITY
+    _LIBCPP_CONSTEXPR_AFTER_CXX17 _LIBCPP_INLINE_VISIBILITY
     ~vector()
----------------
Is this ever constexpr with the calls to the debug functions?


================
Comment at: libcxx/include/vector:653
 
-    bool __invariants() const;
+    _LIBCPP_CONSTEXPR_AFTER_CXX17 bool __invariants() const;
 
----------------
is this function actually constexpr-able?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68365