Make SyntaxTest easier to extend with custom expectations
#14505
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
cameel
force-pushed
the
syntax-test-more-extension-points
branch
from
80511e8 to
f4146ee
Compare
matheusaaguiar
previously approved these changes
Aug 21, 2023
|
|
||
| protected: | ||
| virtual void setupCompiler(); | ||
| void parseAndAnalyze() override; | ||
| virtual void filterObtainedErrors(); | ||
|
|
||
| bool m_optimiseYul = true; | ||
| bool m_parserErrorRecovery = false; | ||
| bool m_optimiseYul{}; |
There was a problem hiding this comment.
I see that m_optimiseYul defaults to true in the constructor, so seems harmless, but still shouldn't we keep it initialized with the same value here too?
There was a problem hiding this comment.
It does not really default to true. It defaults to m_reader.boolSetting("optimize-yul", true), which could end up being either. true is just as misleading here as false.
I think that using default initialization here is the least bad option. It does not show the value so it does not draw attention away from the initialization in the constructor. The actual value here is irrelevant, we only really care that it's initialized to some deterministic value.
|
Needs rebase |
cameel
force-pushed
the
syntax-test-more-extension-points
branch
from
f4146ee to
5fa0993
Compare
|
Rebased. |
cameel
force-pushed
the
syntax-test-more-extension-points
branch
from
5fa0993 to
bf0d51d
Compare
r0qs
approved these changes
Aug 22, 2023
cameel
force-pushed
the
syntax-test-more-extension-points
branch
from
bf0d51d to
7768f26
Compare
- Also reorder Error::Type to match initial values for some consistency
- These get re-initialized in constructor anyway. The only purpose if initializing here is our convention to always initialize primitive types at declaration time. We don't want to have to repeat the defaults though.
…ns in addition to expected errors
cameel
force-pushed
the
syntax-test-more-extension-points
branch
from
7768f26 to
e847596
Compare
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.
Prerequisite for #14433.
I originally planned to derive my new Natspec test case from
TestCasewithAnalysisFrameworkas a mixin. This allowed me to go 90% of the way, but then it turned out that some useful behaviors of our syntax tests are not a reusable part of the base test case, but available only inSyntaxTest:Some of them are in
CommonSyntaxTest, but that in turn does not useAnalysisFrameworkand I'd have to reimplement some of the functionality thatSyntaxTestalready has.This PR modifies
CommonSyntaxTestandSyntaxTestto make it easier to use them as a base for test cases that want to include errors and warnings in their expectations, but also have extra output when analysis succeeds. The Natspec test case will use this mechanism to expect either errors (if Natspec is invalid) or JSON content.