[cfe-commits] r158683 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExpr.cpp test/Sema/inline.c test/SemaCXX/inline.cpp

[John McCall]

On Jun 20, 2012, at 9:13 AM, Jordan Rose wrote:
> On Jun 20, 2012, at 9:00 , Jordan Rose <jordan_rose at apple.com> wrote:
>> On Jun 20, 2012, at 12:51 AM, Chandler Carruth wrote:
>>>> On Mon, Jun 18, 2012 at 3:09 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> Author: jrose
>>>> Date: Mon Jun 18 17:09:19 2012
>>>> New Revision: 158683
>>>> 
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=158683&view=rev
>>>> Log:
>>>> Support -Winternal-linkage-in-inline in C++ code.
>>>> 
>>>> This includes treating anonymous namespaces like internal linkage, and allowing
>>>> const variables to be used even if internal. The whole thing's been broken out
>>>> into a separate function to avoid nested ifs.
>>>> 
>>> This warning seems interesting, and I generally like the correctness insistence, but the current output is really unhelpful. Let's look at one of the *many* cases I'm trying to fix in LLVM's own code:
>>> 
>>> In file included from ../lib/Transforms/Utils/SSAUpdater.cpp:29:
>>> ../include/llvm/Transforms/Utils/SSAUpdaterImpl.h:406:54: warning: function 'PHI_begin' is in an anonymous namespace but is used in an inline method with external linkage [-Winternal-linkage-in-inline]
>>>     for (typename Traits::PHI_iterator I = Traits::PHI_begin(PHI),
>>>                                                    ^
>>> 
>>> So, for starters this message doesn't read naturally with the code snippet its pointing at because the message is a bit inverted. Imagine it instead read as:
>>> 
>>> "warning: calling function 'PHI_begin' from an inline method with external linkage, but this method is defined in an anonymous namespace"
>>> 
>>> This actually starts with the code under the cursor, and takes the user to the nature of the problem.
>> 
>> I see the ordering problem; there's a bit of a dangling reference here (does "this method" refer to PHI_begin or the current method?). I'll try to come up with something else.
> 
> Since the action to take will almost certainly be to change the linkage of the referenced function/variable/method, what do you think about showing the declaration site at the warning and "used here" at the note? It makes the message match the location. The downside is that the message is nowhere near the /use/, so if it's something like Xcode's warnings you'd have to jump to the note to see why this triggered.

This warning really concerns me for a lot of reasons, not least of which being that changing the linkage of the reference function/variable/method is *not* necessarily the appropriate action;  in many cases the appropriate fix might be to change the linkage of the referring code.

More importantly, though, while I understand the instinct of wanting to diagnose violations of the ODR, it is overwhelmingly likely that this is an innocuous violation.  The only way it's *not* innocuous is if we really care about it being the same thing across translation units, e.g. if we're taking its address and expecting it to compare equal, or if it's a mutable variable, or something like that.

For example, the minLinkage function that Chandler fixed was indeed technically an ODR violation, but there's no *bug* there.  There are zero negative consequences to having called this trivially-inlined function from code that's potentially shared across translation units.  I do not see why we should be interested in pedantically diagnosing violations of the ODR that have no negative consequences for our users.

John.