Skip to content

[analyzer][NFC] Trivial cleanup in ArrayBoundChecker #126941

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 1 commit into from
Feb 17, 2025

Conversation

NagyDonat
Copy link
Contributor

Two small stylistic improvements in code that I wrote ~a year ago:

  1. fix a typo in a comment; and
  2. simplify the code of tryDividePair by swapping the true and the false branches.

Two small stylistic improvements in code that I wrote ~a year ago:
1. fix a typo in a comment; and
2. simplify the code of `tryDividePair` by swapping the true and the
   false branches.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Feb 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 12, 2025

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Donát Nagy (NagyDonat)

Changes

Two small stylistic improvements in code that I wrote ~a year ago:

  1. fix a typo in a comment; and
  2. simplify the code of tryDividePair by swapping the true and the false branches.

Full diff: https://github.com/llvm/llvm-project/pull/126941.diff

1 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp (+8-9)
diff --git a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
index 109faacf1726a..f56e9192d1d66 100644
--- a/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
@@ -323,7 +323,7 @@ compareValueToThreshold(ProgramStateRef State, NonLoc Value, NonLoc Threshold,
   // we want to ensure that assumptions coming from this precondition and
   // assumptions coming from regular C/C++ operator calls are represented by
   // constraints on the same symbolic expression. A solution that would
-  // evaluate these "mathematical" compariosns through a separate pathway would
+  // evaluate these "mathematical" comparisons through a separate pathway would
   // be a step backwards in this sense.
 
   const BinaryOperatorKind OpKind = CheckEquality ? BO_EQ : BO_LT;
@@ -394,14 +394,13 @@ static bool tryDividePair(std::optional<int64_t> &Val1,
     return false;
   const bool Val1HasRemainder = Val1 && *Val1 % Divisor;
   const bool Val2HasRemainder = Val2 && *Val2 % Divisor;
-  if (!Val1HasRemainder && !Val2HasRemainder) {
-    if (Val1)
-      *Val1 /= Divisor;
-    if (Val2)
-      *Val2 /= Divisor;
-    return true;
-  }
-  return false;
+  if (Val1HasRemainder || Val2HasRemainder)
+    return false;
+  if (Val1)
+    *Val1 /= Divisor;
+  if (Val2)
+    *Val2 /= Divisor;
+  return true;
 }
 
 static Messages getExceedsMsgs(ASTContext &ACtx, const SubRegion *Region,

Comment on lines 395 to +398
const bool Val1HasRemainder = Val1 && *Val1 % Divisor;
const bool Val2HasRemainder = Val2 && *Val2 % Divisor;
if (!Val1HasRemainder && !Val2HasRemainder) {
if (Val1)
*Val1 /= Divisor;
if (Val2)
*Val2 /= Divisor;
return true;
}
return false;
if (Val1HasRemainder || Val2HasRemainder)
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about removing these boolean variables (and introducing two separate early return paths for Val1 and Val2), but their names seem to be useful for documenting this logic.

@NagyDonat NagyDonat merged commit 6684a59 into llvm:main Feb 17, 2025
11 checks passed
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
Two small stylistic improvements in code that I wrote ~a year ago:
1. fix a typo in a comment; and
2. simplify the code of `tryDividePair` by swapping the true and the
false branches.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants