Skip to content

C++: Add support for more clang builtins #17311

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

Merged
merged 3 commits into from
Aug 28, 2024
Merged

Conversation

jketema
Copy link
Contributor

@jketema jketema commented Aug 27, 2024

No description provided.

@github-actions github-actions bot added the C++ label Aug 27, 2024
@jketema jketema added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 27, 2024
@jketema jketema force-pushed the builtins branch 3 times, most recently from 21a741a to 08afe5d Compare August 28, 2024 08:27
@jketema jketema marked this pull request as ready for review August 28, 2024 10:04
@jketema jketema requested a review from a team as a code owner August 28, 2024 10:04
Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach LGTM - would have expected __datasizeof rather than sizeof in the toString() but perhaps there is a good reason for this.

/** Gets the contained expression. */
Expr getExprOperand() { result = this.getChild(0) }

override string toString() { result = "sizeof(<expr>)" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be __datasizeof

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. A bit too much copy&paste.

/** Gets the contained type. */
Type getTypeOperand() { sizeof_bind(underlyingElement(this), unresolveElement(result)) }

override string toString() { result = "sizeof(" + this.getTypeOperand().getName() + ")" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__datasizeof

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

* A C++ `__datasizeof` expression (used by some implementations
* of the `<type_traits>` header).
*
* The `__datasizeof` expression behaves identical to `sizeof` except
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: identically

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -0,0 +1,10 @@
import cpp

from DatasizeofOperator sto, string elemDesc, Element e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Would it be helpful to also output the constant value of the expression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Also for the sizeof test from which this was copied.

@jketema jketema requested a review from calumgrant August 28, 2024 13:14
Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great!

@jketema jketema merged commit 40fe39c into github:main Aug 28, 2024
15 of 16 checks passed
@jketema jketema deleted the builtins branch August 28, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ depends on internal PR This PR should only be merged in sync with an internal Semmle PR documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants