[libcxx-commits] [PATCH] D125329: Replace modulus operations in std::seed_seq::generate with conditional checks.

[Laramie Leavitt via Phabricator via libcxx-commits]

laramiel marked an inline comment as done.
laramiel added inline comments.


================
Comment at: libcxx/include/__random/seed_seq.h:112
         }
+        size_t __kmodn = 0;          // __k % __n
+        size_t __kpmodn = __p % __n; // (__k + __p) % __n
----------------
Mordante wrote:
> laramiel wrote:
> > Mordante wrote:
> > > This is a kind of clever code which isn't really obvious. Can you add a bit more comment what it exactly does. After you did that the `__k % __n` comment becomes clear.
> > The comments that I added worked to make it clear to me while reading [rand.util.seedseq].
> > What specific comment would make it more clear to you?
> I think it would be nice to explain a bit more what happens and more importantly why the code is written in the current way. That would help future readers to understand the code without looking in the git history. I propose something along the lines of:
> ```
> As an optimization the code in the loop doesn't calculate modulo n every iteration.
> Instead it's initialized here allowing the loop to use an if statement instead of a modulo.
> ```
> 
> If you have a link to the claim of abseil it would be great to add it too.
Ok, I have a very similar comment to that.

Please take another look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125329