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

C++/Java: Share core range analysis #14588

Merged
merged 14 commits into from
Oct 26, 2023

Conversation

aschackmull
Copy link
Contributor

This refactors the core part of range analysis to be shared between C++ and Java. Properly sharing the surrounding support files and further cleanup is left as followup work, as this PR should make sense to merge on its own.

Commit-by-commit review is highly encouraged.

The initial alignment between the two implementations has resulted in a number of minor bug fixes and feature tweaks.

  • C++: Fix broken modulus analysis. The impact of this was assessed separately in C++: Fix modulus analysis. #14413.
  • C++: Fix broken bounds for integer division.
  • C++ and Java: Remove broken bounds for float division (unlikely to have query impact, I think).
  • Java: Fix overflows in QL int (minor bugfix).
  • Java: Proper treatment of long-to-int casts (minor feature addition, no apparent query impact).
  • Java: Add lower bound 0 for AssignAndExpr (small feature addition, a few additional TPs for java/constant-comparison).
  • Java (Kotlin): Disregard some Kotlin-specific casts (very minor tweak, no impact).

Comment on lines +781 to +783
if semNegative(x)
then upper = false and delta = D::fromInt(0)
else none()

Check warning

Code scanning / CodeQL

Use of 'if' with a 'none()' branch. Warning

Use a conjunction instead.
bestBound(e, b, delta, upper)
module Sem implements Semantic {
private import java as J
private import SSA as SSA

Check warning

Code scanning / CodeQL

Names only differing by case Warning

SSA is only different by casing from Ssa that is used elsewhere for modules.
@aschackmull aschackmull requested a review from a team as a code owner October 25, 2023 12:09
@github-actions github-actions bot added the C# label Oct 25, 2023
@aschackmull
Copy link
Contributor Author

DCA looks good.

@aschackmull
Copy link
Contributor Author

I intend to postpone qldoc fixes until I've done some more refactoring. And I also don't want to fix the two remaining ql4ql alerts.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for splitting this up into nice and readable commits ❤️!

@MathiasVP MathiasVP merged commit 30ecb4b into github:main Oct 26, 2023
17 of 18 checks passed
@aschackmull aschackmull deleted the shared/rangeanalysis branch October 27, 2023 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants