[PATCH] D77062: [analyzer] Improved zero assumption in CStringChecke::assumeZero

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

It is a good practice in many projects to make commit messages into imperative (i.e. "Improve" instead of "Improving" or "Improved").  Again, not everyone follows it, but it is good to keep it consistent, right?

@NoQ knock-knock!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:199
   // Utility methods
-  std::pair<ProgramStateRef , ProgramStateRef >
-  static assumeZero(CheckerContext &C,
-                    ProgramStateRef state, SVal V, QualType Ty);
+  std::pair<ProgramStateRef, ProgramStateRef> static assumeZero(
+      ProgramStateRef state, SVal V);
----------------
Can you please put `static` before return type, it will be more consistent with other static methods nearby.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:283-288
+  auto states = std::make_pair(state, state);
+
   Optional<DefinedSVal> val = V.getAs<DefinedSVal>();
-  if (!val)
-    return std::pair<ProgramStateRef , ProgramStateRef >(state, state);
+  // FIXME: We should understand how LazyCompoundVal can be possible here,
+  // fix the root cause and get rid of this check.
+  if (val && !V.getAs<nonloc::LazyCompoundVal>())
----------------
I know that other methods here don't name variables according to llvm-style guide.  Analyzer's code is one of the least complying areas of the whole LLVM project.  However, I suggest not to make it worse and capitalize all parameter and local variable names.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77062/new/

https://reviews.llvm.org/D77062