> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
