r178741 - Add hasExternalLinkageUncached back with the test that Richard provided, but

Rafael EspĂ­ndola rafael.espindola at gmail.com
Thu Apr 4 14:19:51 PDT 2013


> I worry that it's a time bomb -- it bakes in an assumption that we're not
> going to do anything that might change the linkage after we call
> shouldLinkPossiblyHiddenDecl. It also makes the semantics of MergeVarDecl a
> bit weird (it can now succeed without actually merging the declaration with
> a previous decl).  I could live with doing it this way, but I'd like to
> understand the objection to the previous approach first.

I think that anything implementing the current semantics of not
linking a decl if doing so makes it hidden are a bit of a time bomb.
The difference is that this patch is a noisy time bomb. A change that
causes us to call shouldLinkPossiblyHiddenDecl too early will cause an
assert to fail.

A change causing us to call hasExternalLinkageUncached too early can
cause all sort of fun bugs. This was the problem with the old
implementation of getLinkage and we had lots of interesting bugs when
changes (like adding support for auto) caused some call to happen
before the they could.

> I'm also not sure that the changes to CheckOverload are exactly right --
> suppose module M.A declares an external linkage function 'f', module M.B
> imports M.A and contains a using declaration naming 'f', and we then import
> M.B (but not M.A) and provide an invalid external linkage overload of 'f'.
> It looks like we'll ignore M.B's 'f' because we've resolved it to M.A's 'f',
> which is hidden. (I think we should look at whether *I is hidden, not
> whether OldD is.)

I will try to convert this to a test and update the patch with it.

BTW, any comments on the semantics themselves (first part of the
original email)?

Thanks,
Rafael



More information about the cfe-commits mailing list