random.h: add doxygen comments #1683
Conversation
Signed-off-by: Michel van den Hoek <[email protected]>
|
@eboasson Could you please re-run the checks, since this predates the Misra check fix? |
There was a problem hiding this comment.
I haven't figured out how to restart the MISRA check taking into account the changes on master since it ran. Updating this PR might fix that, and fortunately I have found something to nitpick in the comments, to give you a good excuse to push an update 😀 Thanks!
src/ddsrt/include/dds/ddsrt/random.h
Outdated
| * | ||
| * Pseudo random number generator known as the Mersenne Twister. | ||
| * It generates uint32_t from a uniform distribution in the range 0 to 0xffffffff (the maximum value of uint32_t). | ||
| * A useful property is that the use of a fixed seed allows you to reproduce |
There was a problem hiding this comment.
This is true, but it is also the very definition of a PRNG and, I would hope, common knowledge amongst all software developers. So in my view, it should not be in the documentation.
Given that there are a few other changes that I would like to see, perhaps you can remove this, too.
src/ddsrt/include/dds/ddsrt/random.h
Outdated
| @@ -22,23 +31,102 @@ extern "C" { | |||
|
|
|||
| #define DDSRT_MT19937_N 624 | |||
|
|
|||
| /** @brief A composite seed consisting of multiple smaller seeds */ | |||
There was a problem hiding this comment.
It shouldn't be seen as a "composite" seed: it is simply a 256-bit seed.
src/ddsrt/include/dds/ddsrt/random.h
Outdated
| uint32_t ddsrt_prng_random (ddsrt_prng_t *prng); | ||
| size_t ddsrt_prng_random_name(ddsrt_prng_t *prng, char* output, size_t output_size); | ||
| /** | ||
| * @brief Initialize the pseudo random number generator |
There was a problem hiding this comment.
This is not initialising "the PRNG" but rather initialising "the global PRNG", the one that ddsrt_random uses. You can safely init/use your own PRNGs.
I think it would be wise to make this a bit more obvious in the @brief description.
Signed-off-by: Michel van den Hoek <[email protected]>
|
Hi @mvandenhoek I did overlook the update until now, but I can't merge it yet because of the one failing CI build. The logs have been disappeared by Azure, here is a fresh one: https://dev.azure.com/eclipse-cyclonedds/cyclonedds/_build/results?buildId=4629&view=logs&j=f8a8db55-af06-546d-2cb0-cb4efb6f4971&t=a6dfd95d-d3ae-597e-db63-5274130d1879 I'm fine with exporting the handful of additional functions, but the CI checks the actually exported symbols against the list of allowed symbols and that has not been updated yet ... |
Has the CI been updated yet? |
Aside from the comments, I added missing DDS_EXPORT, as I can't think of any good reason to expose 'ddsrt_random' but not the variant that uses a seed.