Add clang-tidy check to catch incorrect use of RNGs #39576
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cur_npc is null exactly when stationed_npcs is empty. Writing it this way makes clang-tidy happier.
Various places in the codebase were not using the standard randomness as intended, which might lead to non-reproducible test failures. Rewrite them to do so. Throw in a litle refactoring at the same time.
We need to ensure that all use of random number generators uses the common tools in rng.h in order to get reproducible test results. Add a clang-tidy check for misuses.
jbytheway
commented
Apr 15, 2020
| @@ -1838,7 +1838,7 @@ void basecamp::job_assignment_ui() | |||
| selection--; | |||
| } | |||
| } else if( action == "CONFIRM" ) { | |||
| if( !stationed_npcs.empty() ) { | |||
| if( cur_npc ) { | |||
There was a problem hiding this comment.
Forgot to mention in the original description: this fix is for an unrelated null pointer dereference warning that clang-tidy also gave me while working on this PR.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
SUMMARY: Infrastructure "Add clang-tidy check to catch incorrect use of RNGs"
Purpose of change
We try to control how randomness enters the game. Primarily this is to permit test failures to be reproducible. Ideally we want the same tests run with the same seed to be identical on every platform.
We haven't achieved that yet, but one thing we need to ensure is that when we're deliberately introducing randomness to the game, we do it in a controlled fashion.
Over the past couple of years we migrated all the randomness to the standardized functions in
rng.h. However, those who are unaware of the constraints are liable to accidentally misuse randomness in ways that are problematic.We need a way to automatically enforce this convention in CI.
Describe the solution
Add a clang-tidy check. It looks for:
rand()and its friends).<random>engine. In most cases, code should use the existing engine, not construct its own.There are a few exceptions, where I have added suppressions for the check.
The other examples I have refactored to use the
rng.hfacilities.Testing
Running clang-tidy and the unit tests.
Additional context
The last major hurdle I'm aware of to portably reproducible tests is the order in which the tests are run. The next step is to switch the tests to
--order randso that our consistent RNG can also control the test order (rather than relying on the linker). But we need the tests to work more reliably when run in a random order before we can do that.