[PATCH] D21675: New ODR checker for modules

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

In https://reviews.llvm.org/D21675#654659, @teemperor wrote:

> I feel like we have a really similar use case in the Analysis/CloneDetection{.h/.cpp} with the hashing of statements. I tried substituting the call to the statement hashing with a call to the CloneDetection API and it seems that most tests pass and the remaining fails are just minor fixable differences (like `::foo()` and `foo()` having different hash codes).
>
> Would be nice if we could make Stmt::Profile, ODRHash and the CloneDetection use the same backend. We improved a few things that we no longer have the periodic `realloc`s from the vector inside NodeIDSet and that we use MD5. Also there are are some future plans on how we can better prevent regressions when we add/extend AST nodes. Thoughts?


It would help to understand your use case better.  Does CloneDetection care about ::foo versus foo?  How about different types with the same name?  ODR checking assumes identical token sequences, so it does care about extra "::".  ODR checking also will process information about type while CloneDetection looks like it only uses the type name.

I see that CloneDetector uses both llvm::MD5 and llvm::FoldingSetNodeID, and only adds data via StringRef.  MD5 will add the bytes as data, while FoldingSetNodeID will add the size of the string, then the bytes as data.  This means MD5 may have collisions when two strings are added back to back while FoldingSetNodeID will store extra data for every integer processed.  FoldingSetNodeID doesn't have this problem if AddInteger is used.  Were the reallocs a big problem for CloneDetection?

I don't thinking merging Stmt::Profile, ODRHash, and CloneDetection would be a good idea unless the hashes needed are very similar.  Stmt::Profile and ODRHash already share a base for Stmt processing, which might be extendable to CloneDetection as well, but a full merge may be difficult.

Sadly, we don't have a good story for when an AST node gets changed with new properties.  The Stmt profiler does declare all the visit methods in the class definition, so that will catch any new Stmt nodes without a new visit method.


https://reviews.llvm.org/D21675