Skip to content

Conversation

@IdrissRio
Copy link
Contributor

@IdrissRio IdrissRio commented Apr 14, 2025

This PR does the following:

  • Creates a new table, type_is_vla.
  • Defines a new predicate, isVla, in the ArrayType class.
  • Modifies the change notes and upgrade/downgrade scripts.

@IdrissRio IdrissRio added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Apr 14, 2025
@IdrissRio IdrissRio force-pushed the idrissrio/vla branch 3 times, most recently from 9d8403e to 3ed4a8d Compare April 14, 2025 08:07
@IdrissRio IdrissRio marked this pull request as ready for review April 14, 2025 10:04
Copilot AI review requested due to automatic review settings April 14, 2025 10:04
@IdrissRio IdrissRio requested a review from a team as a code owner April 14, 2025 10:04
Copy link
Contributor

Copilot AI left a 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 introduces a new predicate, isVla(), in the ArrayType class to enable detection of variable-length arrays. Key changes include:

  • Creating a new table, type_is_vla.
  • Adding the isVla() predicate in ArrayType.
  • Updating change notes and upgrade/downgrade scripts.
Files not reviewed (4)
  • cpp/downgrades/fe54db1af25b6d8a1e25f6cbeae68baf615efc87/upgrade.properties: Language not supported
  • cpp/ql/lib/semmle/code/cpp/Type.qll: Language not supported
  • cpp/ql/lib/semmlecode.cpp.dbscheme: Language not supported
  • cpp/ql/lib/upgrades/e594389175c098d7225683d0fd8cefcc47d84bc1/upgrade.properties: Language not supported

@IdrissRio IdrissRio force-pushed the idrissrio/vla branch 3 times, most recently from 13eb715 to c39dc16 Compare April 14, 2025 12:34
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

One comment, which will unfortunately be a bit of annoying work, otherwise LGTM.

int decl: @stmt_vla_decl ref
);

type_is_vla(unique int type_id: @type ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be a bit painful (because you need to re-do the upgrade scripts, and possibly also the stats file), but you should be able to do the following:

Suggested change
type_is_vla(unique int type_id: @type ref)
type_is_vla(unique int type_id: @derivedtype ref)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! 👍
I'll change that.

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM

@IdrissRio IdrissRio merged commit 67bfe10 into main Apr 16, 2025
16 checks passed
@IdrissRio IdrissRio deleted the idrissrio/vla branch April 16, 2025 09:25
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.

3 participants