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

[Richard Smith]

On Thu, Apr 4, 2013 at 6:45 AM, Rafael Espíndola <rafael.espindola at gmail.com
> wrote:

> > Add hasExternalLinkageUncached back with the test that Richard provided,
> but
> > keep the call at the current location.
>
> I was trying to figure out what these hidden decl are. My
> understanding so far is:
>
> * Modules are handle liked a unit, so loading a module semantically
> loads all sub modules, but they can be hidden.
> * An external variable/function in a hidden sub module still conflicts
> with an external variable/function in the TU importing the module. Is
> this is some form of ODR for modules?
> * An internal variable/function in a hidden sub module doesn't
> interfere with the TU importing the module.
> * An external variable/function in a hidden sub module doesn't
> interfere with a hidden variable/function in the TU importing the
> module.
>
> This last item I find surprising if the objective is really some form
> of ODR. In particular, I would expect an error for f1 and v1 in
> Modules/linkage-merge.m. I would also expect an error in the peculiar
> case Richard mentioned last night:
>
> hidden sub module:
> -------------------------
> extern const int foo;
> ------------------------
> TU importing the module:
> ----------------------------
> const int foo = 32;
> -------------------------
>
> but currently they are not linked and we accept it.
>
> In any case. Even if we want the current semantics, I think we can
> implement them without hasExternalLinkageUncached. We just need to see
> what we are about to link with. At this point if we are linking with a
> hidden declaration, it is safe to compute the linkage of the new one,
> since we know we will only link if the linkage is not change.
>
> The attached patch implements that. Is it OK?


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'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.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130404/5cc438ab/attachment.html>