[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev abandoned this revision.
v.g.vassilev added a comment.

Ok, that's great! Sorry for the delay and thanks for landing a similar patch. 
Btw, we should probably find a more terse way to test the ODRHash, eg. with 
unittests.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



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

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added a comment.

r328404 has been committed to support references and pointers.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



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

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: lib/AST/ODRHash.cpp:581
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }

v.g.vassilev wrote:
> rtrieu wrote:
> > rsmith wrote:
> > > This looks redundant, the above `Visit(const Type*)` function seems to 
> > > already do this.
> > That's correct, VisitType is intended to be empty.  The TypeClass enum 
> > value is added in Visit so that it is the first value added to the data 
> > stream.
> Ok, then I am a little confused. If `VisitType` is supposed to be nop why we 
> call it in all VisitXXX functions.
Each Type calls its parent Type, all the way up to Type.  It's just a manual 
traversal of the Type hierarchy.  Right now, there's nothing in VisitType, but 
there might be in the future.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



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

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added inline comments.



Comment at: lib/AST/ODRHash.cpp:581
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }

rtrieu wrote:
> rsmith wrote:
> > This looks redundant, the above `Visit(const Type*)` function seems to 
> > already do this.
> That's correct, VisitType is intended to be empty.  The TypeClass enum value 
> is added in Visit so that it is the first value added to the data stream.
Ok, then I am a little confused. If `VisitType` is supposed to be nop why we 
call it in all VisitXXX functions.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



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

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: lib/AST/ODRHash.cpp:581
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }

rsmith wrote:
> This looks redundant, the above `Visit(const Type*)` function seems to 
> already do this.
That's correct, VisitType is intended to be empty.  The TypeClass enum value is 
added in Visit so that it is the first value added to the data stream.



Comment at: lib/AST/ODRHash.cpp:590
+  void VisitReferenceType(const ReferenceType *T) {
+AddQualType(T->getPointeeType());
+VisitType(T);

Use T->getPointeeTypeAsWritten() here.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



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

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Looks reasonable, can you add a testcase that shows the ODR checker now 
distinguishes pointers/references to distinct types?




Comment at: lib/AST/ODRHash.cpp:581
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }

This looks redundant, the above `Visit(const Type*)` function seems to already 
do this.


Repository:
  rC Clang

https://reviews.llvm.org/D43696



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

[PATCH] D43696: Reduce hash collisions for reference and pointer types

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev created this revision.
v.g.vassilev added reviewers: rtrieu, rsmith.

While investigating the work done in D41416  I 
found out that the hash values for pointer and reference types are the same.


Repository:
  rC Clang

https://reviews.llvm.org/D43696

Files:
  lib/AST/ODRHash.cpp


Index: lib/AST/ODRHash.cpp
===
--- lib/AST/ODRHash.cpp
+++ lib/AST/ODRHash.cpp
@@ -577,7 +577,19 @@
 Inherited::Visit(T);
   }
 
-  void VisitType(const Type *T) {}
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }
+
+  void VisitPointerType(const PointerType *T) {
+AddQualType(T->getPointeeType());
+VisitType(T);
+  }
+
+  void VisitReferenceType(const ReferenceType *T) {
+AddQualType(T->getPointeeType());
+VisitType(T);
+  }
 
   void VisitAdjustedType(const AdjustedType *T) {
 AddQualType(T->getOriginalType());


Index: lib/AST/ODRHash.cpp
===
--- lib/AST/ODRHash.cpp
+++ lib/AST/ODRHash.cpp
@@ -577,7 +577,19 @@
 Inherited::Visit(T);
   }
 
-  void VisitType(const Type *T) {}
+  void VisitType(const Type *T) {
+ID.AddInteger(T->getTypeClass());
+  }
+
+  void VisitPointerType(const PointerType *T) {
+AddQualType(T->getPointeeType());
+VisitType(T);
+  }
+
+  void VisitReferenceType(const ReferenceType *T) {
+AddQualType(T->getPointeeType());
+VisitType(T);
+  }
 
   void VisitAdjustedType(const AdjustedType *T) {
 AddQualType(T->getOriginalType());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits