Improve object lifetime management when creating temporary Rules object: #4917
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
* Commit 01c37fe introduced a change to the STTx unit test where a local "defaultRules" object was created with a temporary inline "presets" value provided to the ctor. Rules::Impl stores a const ref to the presets provided to the ctor. This particular call provided an inline temp variable, which goes out of scope as soon as the object is created. On Windows, attempting to use the presets (e.g. via the enabled() function) causes an access violation, which crashes the test run. * An audit of the code indicates that all other instances of Rules use the Application's config.features list, which will have a sufficient lifetime.
ximinez
added
Bug
Perf impact not expected
|
ok for 2.1.0: only updates unit test |
Bronek
approved these changes
Feb 14, 2024
intelliot
changed the title
seelabs
approved these changes
Feb 16, 2024
sophiax851
pushed a commit
to sophiax851/rippled
that referenced
this pull request
Jun 12, 2024
* Commit 01c37fe introduced a change to the STTx unit test where a local "defaultRules" object was created with a temporary inline "presets" value provided to the ctor. Rules::Impl stores a const ref to the presets provided to the ctor. This particular call provided an inline temp variable, which goes out of scope as soon as the object is created. On Windows, attempting to use the presets (e.g. via the enabled() function) causes an access violation, which crashes the test run. * An audit of the code indicates that all other instances of Rules use the Application's config.features list, which will have a sufficient lifetime.
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.
High Level Overview of Change
Rulesobjects store a reference to thepresetsprovided to the ctor. One instance in unit tests passed a temporary to the ctor, which resulted in unit tests crashing on Windows on one of my development branches after it was rebased onto 2.1.0-rc1. There doesn't seem to be anything in 2.1.0-rc1 that caused the problem. In fact, when I switch back to changes based on the previous version (2.0.1), it fails, too, despite the CI succeeding.Context of Change
Commit 01c37fe introduced a change to the
STTxunit test where a localdefaultRulesobject was created with a temporary inlinepresetsvalue provided to the ctor.Rules::Implstores a const ref to the presets provided to the ctor. This particular call provided an inline temp variable, which goes out of scope as soon as the object is created. On Windows, attempting to use thepresets(e.g. via theenabled()function) causes an access violation, which crashes the test run.An audit of the code indicates that all other instances of Rules use the Application's
config.featureslist, which will have a sufficient lifetime.Type of Change
API Impact
None