[PATCH] D140859: [clang][dataflow] Allow analyzing multiple functions in unit tests

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp:59
+    if (SM.isPointWithin(Loc, BoundingRange.getBegin(),
+                         BoundingRange.getEnd())) {
+      LineNumberToContent[SM.getPresumedLineNumber(Loc)] =
----------------
ymandel wrote:
> This seems subtly wrong if BoundingRange is a TokenRange. It might be ok, if a comment can't be part of a token (at least, as a prefix), but still seems conceptually wrong in that it's not quite doing what it appears. So, if token ranges are ok, I'd recommend at least explaining that in the comments.
I think it does not matter what kind of range the input range is. The difference is whether the end location is pointing at the beginning or at the end of a token. This span can't contain comments.

Nevertheless, that seemed like subtle reasoning, so I documented the input as a token range, and added a conversion to character range to remove the need for such a subtle justification.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.cpp:89-104
+  unsigned FunctionBeginOffset =
+      SourceManager.getFileOffset(Func->getBeginLoc());
+  unsigned FunctionEndOffset = SourceManager.getFileOffset(Func->getEndLoc());
+
   unsigned I = 0;
-  auto Annotations = AnnotatedCode.ranges();
+  std::vector<llvm::Annotations::Range> Annotations = AnnotatedCode.ranges();
+  llvm::erase_if(Annotations, [=](llvm::Annotations::Range R) {
----------------
sgatev wrote:
> 
Applied.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:173
 llvm::DenseMap<unsigned, std::string>
-buildLineToAnnotationMapping(SourceManager &SM,
+buildLineToAnnotationMapping(SourceManager &SM, SourceRange BoundingRange,
                              llvm::Annotations AnnotatedCode);
----------------
ymandel wrote:
> please specify (in the comments) the intended interpretation of `SourceRange` -- CharRange or TokenRange -- or use `CharSourceRange`.
Documented as a token range, since that's what callers will most likely have.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:177
 /// Runs dataflow specified from `AI.MakeAnalysis` and `AI.PostVisitCFG` on the
 /// body of the function that matches `AI.TargetFuncMatcher` in `AI.Code`.
 /// Given the analysis outputs, `VerifyResults` checks that the results from the
----------------
sgatev wrote:
> "bodies of all functions that match"
Done.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:269
 /// Runs dataflow specified from `AI.MakeAnalysis` and `AI.PostVisitCFG` on the
 /// body of the function that matches `AI.TargetFuncMatcher` in `AI.Code`. Given
 /// the annotation line numbers and analysis outputs, `VerifyResults` checks
----------------
sgatev wrote:
> "bodies of all functions that match"
Done.


================
Comment at: clang/unittests/Analysis/FlowSensitive/TestingSupport.h:367
         VerifyResults(AnnotationStates, AO);
+        AnnotationStates.clear();
       });
----------------
sgatev wrote:
> Can you please add a comment to describe why this needs to be cleared?
Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140859