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.
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
[clang-tidy] The first PR our of many PRs for the "Initialized Class Members" check. #65189
[clang-tidy] The first PR our of many PRs for the "Initialized Class Members" check. #65189
Changes from 2 commits
c6d70f6
ef9c4b6
a0d2270
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
hasDefaultConstructor and unless(has(cxxConstructorDecl())) may opose them self, be more specific, like has user specific constructor....
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.
I don't understand this. Why is
"may opose [sic] them self"
a problem? If they sometimes oppose themselves, the condition is false and the matcher is not added. The important thing is to not oppose themselves always (in which case the condition would always be false), but that is not the case here.We can refine this condition later.
For now, just saying "is default constructor but has no declaration in the AST" is good enough (catches a lot of our cases). We can refine later if we have data (false positives or false negatives).
My understanding is that you find this condition too broad? In that case, that is good. Because that will allow us to see false positives (and filter them out). If the condition would be too narrow, we would have false negatives. False negatives are a problem, because we may not be aware that we have them.
In short, let's leave this as it is for now and we can refine later based on data.
Thanks!
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.
I am not closing this because technically I did not change the code. However, if you think the comment above fully addresses this issue, please close this (if the comment does not fully address, please let me know what you think! --- thanks!)
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.
The thing is that sometimes constructor declarations wont be visible in AST until they actually used first time, this may happen for implicit constructors.
In such case,
hasDefaultConstructor
may return true, butunless(has(cxxConstructorDecl()))
may return false or true, depend if constructor is used or not. I assume that what you wanted to check is to check if default constructor is implicit. Other thing that instead of using "has" it should be "hasMethod".And here you could use things like
unless(has(cxxConstructorDecl(unless(isImplicit()))))
.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.
It'll be good idea to add link to relevant Coding Guidelines.
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.
You are right, fully agree! I added a broader discussion below. To summarize (full context there), we are still working the high level specifications and the low level specifications based on data. Once we have a stable and mature version, we will update the documentation for this checker in a more concrete and actionable way. Until then, we will keep this file (
cpp-init-class-members.rst
) and the unit tests as a high level and low level specifications, respectively. Full details in the broader discussion below. Thanks!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.
I am not closing this because technically I did not change the code. However, if you think the comment above fully addresses this issue, please close this (if the comment does not fully address, please let me know what you think! --- thanks!)