[PATCH] D23418: [analyzer] Added a reusable constraint system to the CloneDetector

[Artem Dergachev via cfe-commits]

NoQ added inline comments.

================
Comment at: include/clang/Analysis/CloneDetection.h:243
@@ +242,3 @@
+    ///         clone groups from the given hash group.
+    virtual bool acceptsHashGroup(const CloneGroup &HashGroup);
+
----------------
I might be wishing a lot, but i've a feeling this interface should be more obvious, i.e. understandable without reading the cpp code.

For example, what is the difference between `acceptsHashGroup` and `acceptsGroup`? I don't think the difference is about accepting different kinds of groups ("hash"-groups vs. "non-hash" groups). If two items X and Y are similar in some sense (and here we have methods with similar names and similar prototypes, and the reader would instantly notice that), it's a good trick to first confirm that these are similar, and then highlight the difference: "However, unlike X, the Y is...".

Another example - are these `accept...` functions supposed to be "pure" (just have a look at the sequence or a group and say if we accept it), or are they supposed to change state of the constraint object? I wish it was obvious from just looking at the interface.

If i wished for even more, i wish this interface could consist of only one method. It seems that this interface is complicated because of performance considerations. So i'd probably want to know more about these considerations, how exactly do all the methods fit together and what is the efficient way of using them.

In order to solve this problem, probably the comment before the class could be extended to explain the big picture. I'd also do my best to understand things better, and probably come up with a wording that would look better from a bystander point of view. Here are a few examples of what seems easier to understand:
- "/// First, CloneDetector filters out groups that are obviously out of place... Then, it takes one prototype from each group and repeatedly asks the Constraint object if other clones of the group are similar to the prototype..."
- "/// If it is obvious just by looking at this group that all sequences in it shouldn't ever be reported, override this callback to notify CloneDetector about that and save some performance..."
- "/// In this callback you may record the property of the prototype you're interested in, so that later you could quickly compare other sequences to it..."


https://reviews.llvm.org/D23418