r178739 - Avoid computing the linkage instead of avoiding caching it.

[Richard Smith]

On Wed, Apr 3, 2013 at 8:27 PM, Rafael Espindola <rafael.espindola at gmail.com
> wrote:

> Author: rafael
> Date: Wed Apr  3 22:27:32 2013
> New Revision: 178739
>
> URL: http://llvm.org/viewvc/llvm-project?rev=178739&view=rev
> Log:
> Avoid computing the linkage instead of avoiding caching it.
>
> This mostly reverts 178733, but keeps the tests.
>
> I don't claim to understand how hidden sub modules work or when we need to
> see
> them (is that documented?), but this has the same semantics and avoids
> adding
> hasExternalLinkageUncached which has the same foot gun potential as the old
> hasExternalLinkage.
>
> Last but not least, not computing linkage when it is not needed is more
> efficient.
>
> Modified:
>     cfe/trunk/include/clang/AST/Decl.h
>     cfe/trunk/lib/AST/Decl.cpp
>     cfe/trunk/lib/Sema/SemaDecl.cpp
>
> Modified: cfe/trunk/include/clang/AST/Decl.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Decl.h?rev=178739&r1=178738&r2=178739&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/AST/Decl.h (original)
> +++ cfe/trunk/include/clang/AST/Decl.h Wed Apr  3 22:27:32 2013
> @@ -219,10 +219,6 @@ public:
>      return getLinkage() == ExternalLinkage;
>    }
>
> -  /// \brief True if this decl has external linkage. Don't cache the
> linkage,
> -  /// because we are not finished setting up the redecl chain for the
> decl.
> -  bool hasExternalLinkageUncached() const;
> -
>    /// \brief Determines the visibility of this entity.
>    Visibility getVisibility() const {
>      return getLinkageAndVisibility().getVisibility();
>
> Modified: cfe/trunk/lib/AST/Decl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/Decl.cpp?rev=178739&r1=178738&r2=178739&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/AST/Decl.cpp (original)
> +++ cfe/trunk/lib/AST/Decl.cpp Wed Apr  3 22:27:32 2013
> @@ -866,10 +866,6 @@ bool NamedDecl::isLinkageValid() const {
>      Linkage(CachedLinkage);
>  }
>
> -bool NamedDecl::hasExternalLinkageUncached() const {
> -  return getLVForDecl(this, LVForExplicitValue).getLinkage() ==
> ExternalLinkage;
> -}
> -
>  Linkage NamedDecl::getLinkage() const {
>    if (HasCachedLinkage)
>      return Linkage(CachedLinkage);
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=178739&r1=178738&r2=178739&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Wed Apr  3 22:27:32 2013
> @@ -1605,19 +1605,6 @@ static void filterNonConflictingPrevious
>    if (previous.empty())
>      return;
>
> -  // If this declaration would have external linkage if it were the first
> -  // declaration of this name, then it may in fact be a redeclaration of
> -  // some hidden declaration, so include those too. We don't need to worry
> -  // about some previous visible declaration giving this declaration
> external
> -  // linkage, because in that case, we'll mark this declaration as a
> redecl
> -  // of the visible decl, and that decl will already be a redecl of the
> -  // hidden declaration if that's appropriate.
> -  //
> -  // Don't cache this linkage computation, because it's not yet correct:
> we
> -  // may later give this declaration a previous declaration which changes
> -  // its linkage.
> -  bool hasExternalLinkage = decl->hasExternalLinkageUncached();
> -
>    LookupResult::Filter filter = previous.makeFilter();
>    while (filter.hasNext()) {
>      NamedDecl *old = filter.next();
> @@ -1627,7 +1614,7 @@ static void filterNonConflictingPrevious
>        continue;
>
>      // If either has no-external linkage, ignore the old declaration.
> -    if (!hasExternalLinkage || old->getLinkage() != ExternalLinkage)
> +    if (old->getLinkage() != ExternalLinkage ||
> !decl->hasExternalLinkage())
>

This isn't correct; this takes us back to caching the linkage of 'decl'
before we set a previous declaration. Testcase attached; build with:

clang++ main.cc -I$PWD/mod -fmodules -fcxx-modules
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130403/31b7fb47/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: r178739.tar
Type: application/x-tar
Size: 10240 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130403/31b7fb47/attachment.tar>