r285289 - [Sema] -Wunused-variable warning for array variables should behave

[Alex L via cfe-commits]

On 31 October 2016 at 15:34, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Thu, Oct 27, 2016 at 6:40 AM Alex Lorenz via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> Author: arphaman
>> Date: Thu Oct 27 08:30:51 2016
>> New Revision: 285289
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=285289&view=rev
>> Log:
>> [Sema] -Wunused-variable warning for array variables should behave
>> similarly to scalar variables.
>>
>> This commit makes the -Wunused-variable warning behaviour more consistent:
>> Now clang won't warn for array variables where it doesn't warn for scalar
>> variables.
>>
>> rdar://24158862
>>
>> Differential Revision: https://reviews.llvm.org/D25937
>>
>> Modified:
>>     cfe/trunk/lib/Sema/SemaDecl.cpp
>>     cfe/trunk/test/SemaCXX/warn-everthing.cpp
>>     cfe/trunk/test/SemaCXX/warn-unused-variables.cpp
>>
>> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/
>> SemaDecl.cpp?rev=285289&r1=285288&r2=285289&view=diff
>> ============================================================
>> ==================
>> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Thu Oct 27 08:30:51 2016
>> @@ -1522,7 +1522,7 @@ static bool ShouldDiagnoseUnusedDecl(con
>>    if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
>>
>>      // White-list anything with an __attribute__((unused)) type.
>> -    QualType Ty = VD->getType();
>> +    const auto *Ty = VD->getType().getTypePtr();
>>
>>      // Only look at the outermost level of typedef.
>>      if (const TypedefType *TT = Ty->getAs<TypedefType>()) {
>> @@ -1535,6 +1535,10 @@ static bool ShouldDiagnoseUnusedDecl(con
>>      if (Ty->isIncompleteType() || Ty->isDependentType())
>>        return false;
>>
>> +    // Look at the element type to ensure that the warning behaviour is
>> +    // consistent for both scalars and arrays.
>> +    Ty = Ty->getBaseElementTypeUnsafe();
>> +
>>      if (const TagType *TT = Ty->getAs<TagType>()) {
>>        const TagDecl *Tag = TT->getDecl();
>>        if (Tag->hasAttr<UnusedAttr>())
>>
>> Modified: cfe/trunk/test/SemaCXX/warn-everthing.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
>> SemaCXX/warn-everthing.cpp?rev=285289&r1=285288&r2=285289&view=diff
>> ============================================================
>> ==================
>> --- cfe/trunk/test/SemaCXX/warn-everthing.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/warn-everthing.cpp Thu Oct 27 08:30:51 2016
>> @@ -9,5 +9,5 @@ public:
>>  };
>>
>>  void testPR12271() { // expected-warning {{no previous prototype for
>> function 'testPR12271'}}
>> -  PR12271 a[1][1]; // expected-warning {{unused variable 'a'}}
>> +  PR12271 a[1][1];
>>  }
>>
>> Modified: cfe/trunk/test/SemaCXX/warn-unused-variables.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
>> SemaCXX/warn-unused-variables.cpp?rev=285289&r1=285288&r2=
>> 285289&view=diff
>> ============================================================
>> ==================
>> --- cfe/trunk/test/SemaCXX/warn-unused-variables.cpp (original)
>> +++ cfe/trunk/test/SemaCXX/warn-unused-variables.cpp Thu Oct 27 08:30:51
>> 2016
>> @@ -150,3 +150,54 @@ namespace ctor_with_cleanups {
>>  }
>>
>>  #include "Inputs/warn-unused-variables.h"
>> +
>> +namespace arrayRecords {
>> +
>> +int total = 0;
>> +
>> +class Adder {
>>
>
> Presumably this class could be a bit simpler - is it just about having a
> non-trivial ctor? or non-trivial dtor?
>
> It'd be helpful to make the test as simple as possible to show what
> features are important to the diagnostic - rather than making the test look
> like real code by having functionality that's not required for testing.
>
> (eg: you don't have to implement functions here - the code will not be
> linked or executed, just compiled for warnings in the test suite)
>
> Potentially name the class by its purpose:
>
> struct NonTriviallyDestructible {
>   ~NonTriviallyDestructible();
> };
>
> and similar.
>

Thanks for looking at this commit!
You're right about this particular class, it can be simpler, since it's
testing a non-trivial door. When I started working on this patch I used the
code from the bug report to reproduce this issue in the test case, and
didn't simplify it further when I found out the cause.


>
> (is it important that Adder and Foo are constructed with arguments/have
> ctor parameters? It's not clear to me from the test or code whether that's
> the case)
>

No. Do you think I should simplify this test case in a separate commit?

Cheers,
Alex


>
>
>> +public:
>> +  Adder(int x); // out of line below
>> +  ~Adder() {}
>> +};
>> +
>> +Adder::Adder(int x) {
>> +  total += x;
>> +}
>> +
>> +struct Foo {
>> +  int x;
>> +  Foo(int x) : x(x) {}
>> +};
>> +
>> +struct S1 {
>> +  S1();
>> +};
>> +
>> +void foo(int size) {
>> +  S1 y; // no warning
>> +  S1 yarray[2]; // no warning
>> +  S1 dynArray[size]; // no warning
>> +  S1 nestedArray[1][2][3]; // no warning
>> +
>> +  Adder scalerInFuncScope = 134; // no warning
>> +  Adder arrayInFuncScope[] = { 135, 136 };  // no warning
>> +  Adder nestedArrayInFuncScope[2][2] = { {1,2}, {3,4} }; // no warning
>> +
>> +  Foo fooScalar = 1; // expected-warning {{unused variable 'fooScalar'}}
>> +  Foo fooArray[] = {1,2}; // expected-warning {{unused variable
>> 'fooArray'}}
>> +  Foo fooNested[2][2] = { {1,2}, {3,4} }; // expected-warning {{unused
>> variable 'fooNested'}}
>> +}
>> +
>> +template<int N>
>> +void bar() {
>> +  Adder scaler = 123; // no warning
>> +  Adder array[N] = {1,2}; // no warning
>> +}
>> +
>> +void test() {
>> +  foo(10);
>> +  bar<2>();
>> +}
>> +
>> +}
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161101/78f36881/attachment.html>