[analyzer] [NFC] another very minor ExprEngineC refactoring

[dcoughlin]

lib/StaticAnalyzer/Core/ExprEngineC.cpp

26 ↗

(On Diff #121426)

I think it would be more clear at the call site what the inputs and output are if this function returned the conjured SVal (or 'Symbol' if it wouldn't be conjured).

It would also be good to describe the role of the parameters in the doxygen comment.

[george.karpenkov]

Addressed review comments.

[george.karpenkov]

@dcoughlin OK to commit?

[dcoughlin]

Yes, looks great. Thanks!

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp

 #include "clang/AST/DeclCXX.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 
 using namespace clang;
 using namespace ento;
 using llvm::APSInt;
 
+/// \brief Optionally conjure and return a symbol for offset when processing
+/// an expression \p Expression.
+/// If \p Other is a location, conjure a symbol for \p Symbol
+/// (offset) if it is unknown so that memory arithmetic always
+/// results in an ElementRegion.
+/// \p Count The number of times the current basic block was visited.
+static SVal conjureOffsetSymbolOnLocation(
+    SVal Symbol, SVal Other, Expr* Expression, SValBuilder &svalBuilder,
+    unsigned Count, const LocationContext *LCtx) {
+  QualType Ty = Expression->getType();
+  if (Other.getAs<Loc>() &&
+      Ty->isIntegralOrEnumerationType() &&
+      Symbol.isUnknown()) {
+    return svalBuilder.conjureSymbolVal(Expression, LCtx, Ty, Count);
+  }
+  return Symbol;
+}
+
 void ExprEngine::VisitBinaryOperator(const BinaryOperator* B,
                                      ExplodedNode *Pred,
                                      ExplodedNodeSet &Dst) {
 
   Expr *LHS = B->getLHS()->IgnoreParens();
   Expr *RHS = B->getRHS()->IgnoreParens();
 
   // FIXME: Prechecks eventually go in ::Visit().
if (Op == BO_Assign) {
                 LeftV, RightV);
       continue;
     }
 
     if (!B->isAssignmentOp()) {
       StmtNodeBuilder Bldr(*it, Tmp2, *currBldrCtx);
 
       if (B->isAdditiveOp()) {
-        // If one of the operands is a location, conjure a symbol for the other
-        // one (offset) if it's unknown so that memory arithmetic always
-        // results in an ElementRegion.
         // TODO: This can be removed after we enable history tracking with
         // SymSymExpr.
         unsigned Count = currBldrCtx->blockCount();
-        if (LeftV.getAs<Loc>() &&
-            RHS->getType()->isIntegralOrEnumerationType() &&
-            RightV.isUnknown()) {
-          RightV = svalBuilder.conjureSymbolVal(RHS, LCtx, RHS->getType(),
-                                                Count);
-        }
-        if (RightV.getAs<Loc>() &&
-            LHS->getType()->isIntegralOrEnumerationType() &&
-            LeftV.isUnknown()) {
-          LeftV = svalBuilder.conjureSymbolVal(LHS, LCtx, LHS->getType(),
-                                               Count);
-        }
+        RightV = conjureOffsetSymbolOnLocation(
+            RightV, LeftV, RHS, svalBuilder, Count, LCtx);
+        LeftV = conjureOffsetSymbolOnLocation(
+            LeftV, RightV, LHS, svalBuilder, Count, LCtx);
       }
 
       // Although we don't yet model pointers-to-members, we do need to make
       // sure that the members of temporaries have a valid 'this' pointer for
       // other checks.
       if (B->getOpcode() == BO_PtrMemD)
         state = createTemporaryRegionIfNeeded(state, LCtx, LHS);