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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits