NoQ added a comment.

Looks awesome!

I'm worried that the test is going to rapidly become stale as you develop 
fixits for `DeclStmt` further. It might make sense to write some //unittests// 
for this class directly, to put into `clang/unittest/Analysis/...`. Then you 
can create some fixits defined by hand, without invoking UnsafeBufferUsage 
analysis, and feed them to this class directly and verify the results.

Also let's explain motivation a bit more. Normally compiler warnings don't need 
such machine because the fixits they come with are very simple and 
hand-crafted. However, unsafe buffer usage analysis is a large emergent 
behavior machine that may fix different parts of user code, and different parts 
of the machine that can produce parts of such fixits aren't necessarily aware 
of each other. In some cases no progress can be made without making sure these 
parts of the machine talk to each other. However, every time the machine does 
emit individual fixits, they're actually correct; conflicts between such fixits 
are entirely a representation problem (we're unable to present overlapping 
fixits to the user because this requires us to resolve the merge conflict), not 
an underlying correctness problem. So @ziqingluo-90 is implementing a bailout 
for the situation when the fixits were perfectly correct in isolation but can't 
be properly displayed to the user due to merge conflicts between them. This 
makes it possible for such "merge conflicts" detection to be purely 
SourceRange-based, so it doesn't need to take the details of the underlying AST 
into account.



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:406
+    const GadgetFix *End = FixIts + NumFixIts;
+    std::vector<GadgetFix> Result;
+    const BitVector &BV = ConflictingFixIts;
----------------
`Result` is unused here!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141338

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to