-
Notifications
You must be signed in to change notification settings - Fork 1.8k
C++: Lift getTypeOperand to a superclass
#20864
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
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.
Pull Request Overview
This PR lifts the getTypeOperand() method from the SizeofTypeOperator subclass to the SizeofOperator superclass, making it available for both type and expression operands. For sizeof(myExpr), the method now returns the type of the expression. The same transformation is applied to the DatasizeofOperator hierarchy.
Key Changes:
- Added
getTypeOperand()toSizeofOperatorandDatasizeofOperatorbase classes with default implementation{ none() } - Implemented overrides in expression operator subclasses to return the type of the contained expression
- Updated test to call the method on the superclass, demonstrating it now works for all sizeof variants
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cpp/ql/lib/semmle/code/cpp/exprs/Cast.qll | Added getTypeOperand() to superclasses SizeofOperator and DatasizeofOperator, with appropriate overrides in subclasses |
| cpp/ql/test/library-tests/types/sizeof/sizeof.ql | Updated test to call getTypeOperand() on SizeofOperator instead of SizeofTypeOperator |
| cpp/ql/test/library-tests/types/sizeof/sizeof.expected | Updated expected results to include type operands for expression-based sizeof operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
That's a pretty recent addition actually. |
jketema
left a comment
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.
LGTM if CI and DCA are happy.
|
DCA was uneventful. Merging! |
Both
sizeof(myType)andsizeof(myExpr)exists in C/C++, and we currently have a class for each case (SizeofTypeOperatorandSizeofExprOperator, respectively) with a common superclassSizeofOperator.This PR lifts the
getTypeOperandmember predicate fromSizeofTypeOperatorto the superclass with the intuitive implementation that, in the case ofsizeof(myExpr)it should return the type ofmyExpr. This makes the superclass much more useful.:til:there's also a__datasizeofin Clang so I made a similar transformation on that one.