[cfe-commits] r166361 - in /cfe/trunk: lib/CodeGen/CGExpr.cpp lib/Sema/SemaExpr.cpp test/CodeGenCXX/const-init-cxx11.cpp test/CodeGenCXX/for-range.cpp test/CodeGenCXX/lambda-expressions.cpp test/SemaCXX/lambda-expressions.cpp

[Argyrios Kyrtzidis]

On Oct 26, 2012, at 3:12 PM, Nico Weber <thakis at chromium.org> wrote:

> On Fri, Oct 26, 2012 at 2:19 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>> On Oct 26, 2012, at 2:16 PM, Nico Weber <thakis at chromium.org> wrote:
>> 
>>> FWIW, I found -Wunneeded-internal-declaration fairly confusing. I
>>> tried to turn it on for chromium, and it fired in a few instances
>>> where the function it warned about was used for something and couldn't
>>> be easily removed.
>> 
>> Could you be more specific ? It's not clear if you are talking about a false positive or something else.
> 
> I don't remember details. I did a new build with the flag right now,
> here's what it found:
> 
> 
> In this case, clang warns about _NullPool which is used in the
> function Null() which in turn is used in some translation units:
> 
> http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/harfbuzz-ng/src/hb-open-type-private.hh&exact_package=chromium&q=_NullPool&l=134
> http://code.google.com/p/chromium/source/search?q=file%3Aharfbuzz+Null%5C%28&origq=file%3Aharfbuzz+Null%5C%28&btnG=Search+Trunk
> 
> ../../third_party/harfbuzz-ng/src/hb-open-type-private.hh:136:20:
> error: variable '_NullPool' is not needed and will not be emitted
> [-Werror,-Wunneeded-internal-declaration]
> static const void *_NullPool[64 / sizeof (void *)];
>                   ^
> 1 error generated.
> 
> I'm not sure what to do with this warning.

It says so in the comment above it :-)

> /* TODO This really should be a extern HB_INTERNAL and defined somewhere... */

We could have the warning suppressed for static variables in headers though.

> 
> 
> 
> A related example: clang thinks that GrInitialArrayAllocationCount()
> should be "static inline" (ok, I guess). However,
> GrNextArrayAllocationCount() right below it doesn't warn, even though
> the two functions seem to be used in the same way later in the file
> (in GrTDArray::growAt()).
> 
> http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/skia/src/gpu/GrTDArray.h&exact_package=chromium&q=GrInitialArrayAllocationCount&l=17
> 
> In file included from ../../third_party/skia/src/gpu/gl/GrGpuGL.cpp:9:
> In file included from ../../third_party/skia/src/gpu/gl/GrGpuGL.h:23:
> In file included from ../../third_party/skia/src/gpu/gl/../GrTHashCache.h:14:
> ../../third_party/skia/src/gpu/GrTDArray.h:17:12: error: 'static'
> function 'GrInitialArrayAllocationCount' declared in header file
> should be declared 'static inline'
> [-Werror,-Wunneeded-internal-declaration]
> static int GrInitialArrayAllocationCount() {
>           ^
> 1 error generated.

I assume you mean there's a false negative here ? Is there no "unused function" warning ? I suggest filing a bug with a preprocessed file.

-Argyrios

> 
> 
> 
> …and that's all already. The last time I tried I think there were more issues.
> 
> Nico
> 
>> 
>> -Argyrios
>> 
>>> 
>>> Nico
>>> 
>>> On Fri, Oct 26, 2012 at 1:54 PM, Argyrios Kyrtzidis <kyrtzidis at apple.com> wrote:
>>>> On Oct 26, 2012, at 1:35 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>>>> 
>>>>> Argyrios: It looks like you added this warning in r129794. Can you
>>>>> comment on what it's intended to detect?
>>>> 
>>>> variables/functions with internal linkage that are not used from the codegen perspective.
>>>> This differs from -Wunused which will consider a 'use' even in an unevaluated context.
>>>> For example:
>>>> 
>>>> static void foo() { }
>>>> 
>>>> this gives:
>>>> warning: unused function 'foo' [-Wunused-function]
>>>> 
>>>> static void foo() { }
>>>> template <typename T>
>>>> void goo() {
>>>> foo();
>>>> }
>>>> 
>>>> this gives:
>>>> warning: function 'foo' is not needed and will not be emitted [-Wunneeded-internal-declaration]
>>>> 
>>>> -Argyrios
>>>> 
>>>>> 
>>>>> On Fri, Oct 26, 2012 at 9:52 AM, Craig Topper <craig.topper at gmail.com> wrote:
>>>>>> No its used right here in the same file. It only warns on C++11 enabled
>>>>>> builds.
>>>>>> 
>>>>>>  virtual void getAnalysisUsage(AnalysisUsage &AU) const {
>>>>>>    AU.setPreservesAll();
>>>>>>    AU.addRequiredID(PreVerifyID);
>>>>>> 
>>>>>> 
>>>>>> On Fri, Oct 26, 2012 at 9:41 AM, Matthieu Monrocq
>>>>>> <matthieu.monrocq at gmail.com> wrote:
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On Fri, Oct 26, 2012 at 8:51 AM, Richard Smith <richard at metafoo.co.uk>
>>>>>>> wrote:
>>>>>>>> 
>>>>>>>> On Thu, Oct 25, 2012 at 10:48 PM, Craig Topper <craig.topper at gmail.com>
>>>>>>>> wrote:
>>>>>>>>> I think this change broke bootstrap builds with C++11 and -Werror
>>>>>>>>> enabled.
>>>>>>>>> 
>>>>>>>>> lib/VMCore/Verifier.cpp:116:14: error: variable 'PreVerifyID' is not
>>>>>>>>> needed
>>>>>>>>> and will not be emitted [-Werror,-Wunneeded-internal-declaration]
>>>>>>>>> static char &PreVerifyID = PreVerifier::ID;
>>>>>>>> 
>>>>>>>> Yes, I think this change probably caused that. I'm unclear on what the
>>>>>>>> purpose of that warning is: it appears to be warning on variables
>>>>>>>> which are referenced but not odr-used, which seems like a pretty
>>>>>>>> questionable thing to warn on. It'd be easy enough to fix this by
>>>>>>>> teaching Sema::ShouldWarnIfUnusedFileScopedDecl to ignore references
>>>>>>>> (along with its existing check for const variables), but I'm not sure
>>>>>>>> that's the right fix, since I'm not really sure what the intent is
>>>>>>>> here.
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> Is not PreVerifyID just an unused variable ? (not the difference with
>>>>>>> PreVerifier::ID)
>>>>>>> 
>>>>>>> -- Matthieu
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> --
>>>>>> ~Craig
>>>> 
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20121026/e39e9192/attachment.html>