[PATCH] D21675: New ODR checker for modules

[Richard Trieu via cfe-commits]

rtrieu added a comment.

In https://reviews.llvm.org/D21675#560297, @rsmith wrote:

> There are a bunch of cases here that do this:
>
>   if (auto t = getThing())
>     ID.addThing(t);
>   if (auto t = getOtherThing())
>     ID.addThing(t);
>
>
> That will result in hash collisions between objects with thing and objects with otherthing (for instance, `struct A { int n : 1; };` and `struct A { int n = 1; };` appear to hash the same).
>
> And there are some cases where you add a list of objects without first adding the size, which is liable to allow collisions.
>
> I'm not too worried about these cases, since this is just a hash anyway, and is only a best-effort attempt to check for ODR violations, but some of them look like they'd be easy enough to fix, so I figure why not :)


The hashing now does:

  auto t = getThing();
  ID.addBoolean(t);
  if (t)
    ID.addThing(t);

The added Boolean value prevents the hash collision.

> Anyway, this looks really good. Do you know if the computation of the ODR hash has any measurable performance impact when building a large module? I'm not really expecting one, but it seems worth checking just in case.

I ran some comparisons and did not see any performance impact.



================
Comment at: lib/AST/DeclBase.cpp:1827
+  void VisitParmVarDecl(const ParmVarDecl *D) {
+    VisitStmt(D->getDefaultArg());
+    Inherited::VisitParmVarDecl(D);
----------------
rsmith wrote:
> You should not include the default argument if it was inherited from a previous declaration. That can happen in a friend declaration:
> ```
> // module 1
> void f(int = 0);
> struct X { friend void f(int); }; // has (inherited) default arg
> ```
> ```
> // module 2
> struct X { friend void f(int); }; // has no default arg
> ```
The visitor for friend decl only uses the type (void (int)) and name (f) of the friend.  It doesn't process the type, so default arguments doesn't matter for this case.


https://reviews.llvm.org/D21675