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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const 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));
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68365