[PATCH] D40731: Integrate CHash into CLang

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.
Herald added a subscriber: JDevlieghere.


================
Comment at: include/clang/AST/CHashVisitor.h:72-79
+  template<typename T>
+  void addData(const llvm::iterator_range<T> &x) {
+    addData(std::distance(x.begin(), x.end()));
+  }
+  template<typename T>
+  void addData(const llvm::ArrayRef<T> &x) {
+    addData(x.size());
----------------
Should these also be storing the hashes of the elements in addition to storing the size?


================
Comment at: include/clang/AST/CHashVisitor.h:211
+
+  bool VisitTypeDecl(TypeDecl *D) {
+    // If we would hash the resulting type for a typedef, we
----------------
I thought the idea was to have all the code in the *Collectors.td but here you have a bunch of Visit* functions for AST nodes.  This seems counter to your point of having them generated.


================
Comment at: include/clang/AST/CHashVisitor.h:212-213
+  bool VisitTypeDecl(TypeDecl *D) {
+    // If we would hash the resulting type for a typedef, we
+    // would get into an endless recursion.
+    if (!isa<TypedefNameDecl>(D) && !isa<RecordDecl>(D) && !isa<EnumDecl>(D)) {
----------------
I don't see this example in the test.


================
Comment at: include/clang/AST/CHashVisitor.h:226
+    }
+    if (isa<VarDecl>(ValDecl)) {
+      /* We emulate TraverseDecl here for VarDecl, because we
----------------
Can't you query the hash for the Decl instead of doing work here?


================
Comment at: include/clang/AST/CHashVisitor.h:253
+  bool VisitValueDecl(ValueDecl *D) {
+    /* Field Declarations can induce recursions */
+    if (isa<FieldDecl>(D)) {
----------------
Is this possible recursion included in your test?


================
Comment at: include/clang/AST/CHashVisitor.h:255
+    if (isa<FieldDecl>(D)) {
+      addData(std::string(D->getType().getAsString()));
+    } else {
----------------
Shouldn't this be handled in the VisitFieldDecl function?


================
Comment at: include/clang/AST/DeclDataCollectors.td:5-7
+      // Every Declaration gets a tag field in the hash stream. It is
+      // hashed to add additional randomness to the hash
+      addData(llvm::hash_value(S->getKind()));
----------------
Why is this randomness needed?


================
Comment at: include/clang/AST/DeclDataCollectors.td:9
+
+      // CrossRef
+      addData(S->hasAttrs());
----------------
What do you mean by "CrossRef"?


================
Comment at: include/clang/AST/DeclDataCollectors.td:60-61
+  code Code = [{
+       /* Not every enum has a init expression. Therefore, 
+          we extract the actual enum value from it. */
+       addData(S->getInitVal().getExtValue());
----------------
Does that mean you consider:

```
enum E { E1, E2, E3 };
```
to be equivalent to:

```
enum E { E1 = 0, E2 = 1, E3 = 2 };
```
?


Repository:
  rC Clang

https://reviews.llvm.org/D40731