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

[Jim Grosbach]

I'm seeing many, many warnings compiling LLVM with this warning, in particular from things like the ctype macros (isascii(), isupper(), et. al.). The common theme is static inline functions in headers. Consider:

$ cat fail.h
static inline int foo() { return 0; }

class test {
public:
  int blah() const {
    return foo();
  }
};
$ cat fail.cpp
#include "fail.h"
$ clang++ fail.cpp -c
In file included from fail.cpp:1:
./fail.h:6:12: warning: function 'foo' has internal linkage but is used in an inline method with external linkage [-Winternal-linkage-in-inline]
    return foo();
           ^
./fail.h:1:19: note: 'foo' declared here
static inline int foo() { return 0; }
                  ^
1 warning generated.

This strikes me as a false positive.

-Jim

On Jun 20, 2012, at 10:51 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Jun 20, 2012, at 10:45 , John McCall <rjmccall at apple.com> wrote:
> 
>> 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:
>>> 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.
> 
> Oops, yes, the usual fix (in C) is to insert "static".
> 
> 
>> 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.
> 
> There would indeed be cases where this is in fact incredibly dangerous, and I'm sad we can't warn about them, but maybe you're right that it's just not something we can diagnose effectively. Any warning that people will eventually just turn off is probably not worth keeping around. (Off-by-default? …)
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits