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

[Jordan Rose]

On Jun 18, 2012, at 16:21 , Richard Smith <richard at metafoo.co.uk> wrote:

> On Mon, Jun 18, 2012 at 4:06 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>> On Jun 18, 2012, at 15:42 , Richard Smith <richard at metafoo.co.uk> wrote:
>> 
>>> On Mon, Jun 18, 2012 at 3:28 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> 
>>>> On Jun 18, 2012, at 15:23 , Eli Friedman <eli.friedman at gmail.com> 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.
>>>>> 
>>>>> I think it's worth pointing out that in the C++ case, the given
>>>>> testcase doesn't strictly violate ODR because the  definition of the
>>>>> function in question isn't actually used in multiple files.  Because
>>>>> of that, it shouldn't be an error with -pedantic-errors (the
>>>>> diagnostic should use Warning rather than Extension/ExtWarn), and you
>>>>> should watch to see if there are any bug reports with false positives.
>>>>> (I think false positives are unlikely, but not impossible.)
>>>> 
>>>> Ah, I see. Without cross-TU analysis, we can't tell if a function is used in multiple files or not. I think it's valid to leave this as ExtWarn when it's in a header file…it's kind of a ticking time bomb.
>>> 
>>> -pedantic-errors shouldn't cause us to reject valid code, and not all
>>> #included files are intended to be included multiple times (we have
>>> several examples of this in Clang, with (for instance) files generated
>>> by tablegen).
>>> 
>>> Have you considered implementing the check for whether a variable used
>>> within such an inline function has a literal type (or, in C++98, an
>>> integral or enumeration type), and checking whether the initializer is
>>> a constant expression?
>>> 
>>> Checking whether the variable's address is used seems trickier,
>>> perhaps we can use the result of the odr-use checking mechanism?
>> 
>> I skipped the literal type check; currently it allows all const variables with initializers, literal or not. There's also no check for the address clause (which I mistakenly interpreted as equivalent to "prvalue only", but which allows references). Those checks won't cause us to reject valid code.
>> 
>> The only valid code we will reject under -pedantic-errors here is code that references internal linkage / anonymous namespace non-constant variables or functions from a file that is included in one translation unit in the entire compilation. From the perspective of perfection, this is technically incorrect, and for -pedantic-errors that may be what we want. But there is never a case where this is not fixable (by putting the offending function/method in an anonymous namespace), and if someday the included file shows up in two translation units you have a legitimate pedantic-error which could affect the behavior of your program.
>> 
>> I'm genuinely not sure which is worse: missing an error because we don't do cross-TU checking, or preventing compilation on the case where something is only included once. We'd warn whether it's ExtWarn or Warning, so that's not the issue.
> 
> Changing ExtWarn -> Warning seems to have no downside. It just makes
> -pedantic-errors more pedantically correct, which is, after all, the
> point. If someone is using -pedantic-errors but not -Werror, they will
> get what they asked for -- allowing this code is not an extension,
> after all.

Okay, got it. Thanks, Richard and Eli. Will change.