[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

[Nico Weber]

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.



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.



…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
>