[cfe-commits] r103212

[Ted Kremenek]

Thanks Richard.  It's embarrassing to see this code after you point it out, but I can see with the complexity of the switch logic how easy it was to happen.  I'm curious to know how you found it.  Manual code audit, or something else?

In the spirit of the original change, it appears the correct fix would be to add 'return true' in the 'InternalLinkage' case.  That may have been true at one time, but now it's not clear if we should not be generating USRs for cursors with InternalLinkage.  Indeed, putting in the fix I suggest breaks several of our tests in ways that aren't obvious if they matter to some clients.  Much has changed on libclang since I wrote that code, and I'm not really the owner of that anymore and haven't' looked at this in a long time.  I've gone and removed that logic in r167442, since it's effectively dead code, but this logic should get re-reviewed.

On Nov 5, 2012, at 5:39 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> Hi Ted,
> 
> Revision 103212 has its 2.5th birthday today. It introduced a bug:
> 
> Index: tools/libclang/CIndexUSRs.cpp
> ===================================================================
> --- tools/libclang/CIndexUSRs.cpp       (revision 103211)
> +++ tools/libclang/CIndexUSRs.cpp       (revision 103212)
> [...]
> @@ -383,6 +394,7 @@
>         // Generate USRs for all entities with external linkage.
>         break;
>       case NoLinkage:
> +      case UniqueExternalLinkage:
>         // We allow enums, typedefs, and structs that have no linkage to
>         // have USRs that are anchored to the file they were defined in
>         // (e.g., the header).  This is a little gross, but in principal
> @@ -390,14 +402,12 @@
>         // are referred to across multiple translation units.
>         if (isa<TagDecl>(ND) || isa<TypedefDecl>(ND) ||
>             isa<EnumConstantDecl>(ND) || isa<FieldDecl>(ND) ||
> -            isa<VarDecl>(ND))
> +            isa<VarDecl>(ND) || isa<NamespaceDecl>(ND))
>           break;
>         // Fall-through.
>       case InternalLinkage:
>         if (isa<FunctionDecl>(ND))
>           break;
> -      case UniqueExternalLinkage:
> -        return createCXString("");
>     }
> 
>   StringUSRGenerator SUG(&C);
> 
> Note that we had fall-through into the 'return' statement before, and
> now it's gone. This entire 'switch' statement just executes a bunch of
> tests, then either 'break's or falls off the bottom. That can't be
> what was intended...