[PATCH] D143751: [clang][analyzer][NFC] Refactor code of StdLibraryFunctionsChecker.

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

A high level comment before getting into (even more) nitty gritty stuff. But shut me down if I misunderstood whats happening.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:124-125
   // requirement would render the initialization of the Summary map infeasible.
+  // A new value constraint is created furthermore by the negate functionality
+  // of the constraint and returned as pointer.
   using ValueConstraintPtr = std::shared_ptr<ValueConstraint>;
----------------
balazske wrote:
> Szelethus wrote:
> > Is this what you meant?
> Probably this is better:
> ```
>   // Mind that a pointer to a new value constraint can be created by negating an existing
>   // constraint.
> ```
> The important thing is that one reason for the shared pointer is the negate function that returns a pointer.
Can be, or //will// be? But otherwise, sure.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:271
+
+  private:
+    using RangeApplyFunction =
----------------
balazske wrote:
> This new private section is the new added code.
While generalizing code is great, whenever its done by introducing function arguments / lambdas, the code becomes harder to understand. This is fine, as long as this complexity is justified, but I really needed to see what happened in the followup patch to see whats the gain behind this.

The gain is that you can capture a stream and construct a helpful message as the range is applied. 

Question: couldn't you just expose a lambda for the specific purpose for string smithing, and only that? Seems like all lambdas contain kind of the same thing: a call to `ConstraintManager::assumeInclusiveRange`. 

Maybe this design (for the above reasons) is worth documenting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143751