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