-
Notifications
You must be signed in to change notification settings - Fork 1.5k
some Tokenizer construction and related other cleanups
#4799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Tokenizer construction cleanupsTokenizer construction and related other cleanups
| } | ||
|
|
||
| void checkNoMemset_(const char* file, int line, const char code[], const Settings &settings) { | ||
| void checkNoMemset_(const char* file, int line, const char code[], Settings &settings) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is settings changed? then I'd prefer to pass it by pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will be addressed by #4798 and subsequent changes.
And we should/need to get rid of pointers and not introduce new ones. They are essentially outlawed in C++20 with being replaced by various constructs and containers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it used to work with a const reference but now you add a preprocessor object then it does not work anymore. Is it required to add the preprocessor here?
I think it would be better if the Preprocessor argument would not be a Settings& settings. I would suggest it's changed to a pointer or something.
I find it dangerous when there is so much changes and there can be subtle issues here and there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the Preprocessor changes the Settings. That's why they cannot be shared between tests and we need to create a fresh one or properly reset them between tests. That will be changed in the referenced PR to make all objects const unless they need to be and then make them local ones that do not cause modifications to still into other tests.
It's "iterative" (actually the following PR is quite big but follow ups will be smaller).
I find it dangerous when there is so much changes and there can be subtle issues here and there.
Actually there's a lot of subtle issues which are obfuscated by all the pointers and non-const and such. Those will be all gone after the refactoring is done.
When
Preprocessorwas not set inTokenizerit was bypassing some logic. We should not be doing that as it might cause tests to behave differently than production code.The
static Settingsare dangerous as the objects are mutable. #4798 will start to address that.TestTypewas also using the settings quite inconsistently. There's obviously some room for cleanups.