Skip to content
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

Rust: Change array element content type into a general collection element content type #18568

Merged
merged 3 commits into from
Jan 24, 2025

Conversation

paldepind
Copy link
Contributor

This new CollectionContent is similar to the same content type in C/C++ and Swift.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Jan 23, 2025
@paldepind paldepind closed this Jan 23, 2025
@paldepind paldepind reopened this Jan 23, 2025
@paldepind paldepind marked this pull request as ready for review January 23, 2025 09:06
@Copilot Copilot bot review requested due to automatic review settings January 23, 2025 09:06

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@paldepind paldepind requested a review from geoffw0 January 23, 2025 09:07
*/
final class ArrayElementContent extends Content, TArrayElement {
ArrayElementContent() { this = TArrayElement() }
final class CollectionContent extends Content, TArrayElement {
Copy link
Contributor

Choose a reason for hiding this comment

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

TArrayElement should be renamed to TElementContent as well. This will also reveal that the MaD token "ArrayElement" needs to be changed in FlowSummaryImpl.qll. And once that is done, the QL doc in ModelsAsData.qll needs to be updated, as well as a few tests.

geoffw0
geoffw0 previously approved these changes Jan 23, 2025
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM. I have a local branch that is waiting for this, thanks for getting it done!

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

One last thing, otherwise LGTM.

rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll Outdated Show resolved Hide resolved
@paldepind paldepind merged commit b84adec into github:main Jan 24, 2025
15 checks passed
@paldepind paldepind deleted the rust-container branch January 24, 2025 08:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants