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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

LGTM after adding the comment. Do you need somebody to land to patch for you?
If so please provide your name and e-mail address.



================
Comment at: libcxx/include/__random/seed_seq.h:112
         }
+        size_t __kmodn = 0;          // __k % __n
+        size_t __kpmodn = __p % __n; // (__k + __p) % __n
----------------
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.


================
Comment at: libcxx/include/__random/seed_seq.h:151
         }
         for (size_t __k = __m; __k < __m + __n; ++__k)
         {
----------------
laramiel wrote:
> Mordante wrote:
> > I think here we can add some comment too. The modulo in this loop works when `__k == __m`, which is true when `__s + 1 <= __m`.
> What comment do you think helps? I think that the modulo pattern is pretty clear, particularly since it mirrors the first loop.
I see the precondition `__s + 1 <= __m` is always true. Might be nice to add a comment to that effect, but not required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125329