[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D100581#3404624 , @xbolva00 wrote:

> @mbenfield found false positive
>
>   void test(void) {
> static int counter = 0;
> counter += 5;
>   }

posted https://reviews.llvm.org/D122374


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.
Herald added subscribers: pcwang-thead, luke957.
Herald added a project: All.

@mbenfield found false positive

  void test(void) {
static int counter = 0;
counter += 5;
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D100581#2798079 , @raj.khem wrote:

> http://sprunge.us/FJzZXL is a file from harfbuzz and it warns
>
>   a.cc:28670:32: error: variable 'supp_size' set but not used 
> [-Werror,-Wunused-but-set-variable]
>   unsigned int size0, size1, supp_size = 0;

You have, with pragma gcc error, look at hb.hh. The problem is that clang has 
an error dealing with cascading pragma when setting different levels.

> I do not have -Werror enabled but it still is reported as error. There is no 
> way to disable this warning ?

Go to the hb GitHub repo, I’ve fixed this in master branch, it will propagate 
soon to a new release.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D100581#2798079 , @raj.khem wrote:

> http://sprunge.us/FJzZXL is a file from harfbuzz and it warns
>
>   a.cc:28670:32: error: variable 'supp_size' set but not used 
> [-Werror,-Wunused-but-set-variable]
>   unsigned int size0, size1, supp_size = 0;
>
> I do not have -Werror enabled but it still is reported as error. There is no 
> way to disable this warning ?

Well, you have

>> #pragma GCC diagnostic error "-Wunused"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Khem Raj via Phabricator via cfe-commits]

raj.khem added a comment.

http://sprunge.us/FJzZXL is a file from harfbuzz and it warns

  a.cc:28670:32: error: variable 'supp_size' set but not used 
[-Werror,-Wunused-but-set-variable]
  unsigned int size0, size1, supp_size = 0;

I do not have -Werror enabled but it still is reported as error. There is no 
way to disable this warning ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2795966 , @sberg wrote:

> Is it intentional that this warns about volatile variables as in

Yes, volatile was intentional. Alright, I will add a test for this case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Stephan Bergmann via Phabricator via cfe-commits]

sberg added a comment.

Is it intentional that this warns about volatile variables as in

  void f(char * p) {
  volatile char c = 0;
  c ^= *p;
  }

(I see that GCC warns about volatile too, at least when you replace the `^=` 
with `=`, so assume the answer is "yes", but would just like to see that 
confirmed (ideally with a test case even?).)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2792854 , @Abpostelnicu 
wrote:

> I think there is a false positive with this @george.burgess.iv:
> In this 
> 
>  we have the warning triggered: 
> mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 
> 'r' set but not used [-Werror,-Wunused-but-set-variable]
>
>   SigHandlerCoordinator() {
> PodZero();
> int r = sem_init(, /* pshared */ 0, 0);
> r |= sem_init(, /* pshared */ 0, 0);
> r |= sem_init(, /* pshared */ 0, 0);
> MOZ_ASSERT(r == 0);
>   }

Indeed as pointed out above I think this is a true positive unfortunately.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D100581#2793926 , @dblaikie wrote:

> In D100581#2793775 , @ldionne wrote:
>
>> Hello! There are still some false positives, for example this one is 
>> breaking the libc++ CI: 
>> https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848
>>
>> Would it be possible to either fix this quickly or revert the change until 
>> the issue has been fixed? Our pre-commit CI is going to be stalled until 
>> this is fixed. Thanks!
>
> Looks like a true positive in libc++ ( 
> https://github.com/llvm/llvm-project/blob/main/libcxx/include/regex ) - the 
> "__j" variable is initialized, then incremented, but never read (except to 
> increment). Is that a bug in libc++? Was __j meant to be used for something?

Oh, you're right! I was tripped by the fact that we did in fact "use" `__j` (in 
the sense that we do `__j += ...`), but indeed we never read the result. I'll 
work on fixing that issue. I'm not sure whether it's a bug or not yet, that 
code was modified 11 years ago, but I'll do my research.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100581#2793775 , @ldionne wrote:

> Hello! There are still some false positives, for example this one is breaking 
> the libc++ CI: 
> https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848
>
> Would it be possible to either fix this quickly or revert the change until 
> the issue has been fixed? Our pre-commit CI is going to be stalled until this 
> is fixed. Thanks!

Looks like a true positive in libc++ ( 
https://github.com/llvm/llvm-project/blob/main/libcxx/include/regex ) - the 
"__j" variable is initialized, then incremented, but never read (except to 
increment). Is that a bug in libc++? Was __j meant to be used for something?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

Hello! There are still some false positives, for example this one is breaking 
the libc++ CI: 
https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a8848

Would it be possible to either fix this quickly or revert the change until the 
issue has been fixed? Our pre-commit CI is going to be stalled until this is 
fixed. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D100581#2792854 , @Abpostelnicu 
wrote:

> I think there is a false positive with this @george.burgess.iv:
> In this 
> 
>  we have the warning triggered: 
> mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 
> 'r' set but not used [-Werror,-Wunused-but-set-variable]
>
>   SigHandlerCoordinator() {
> PodZero();
> int r = sem_init(, /* pshared */ 0, 0);
> r |= sem_init(, /* pshared */ 0, 0);
> r |= sem_init(, /* pshared */ 0, 0);
> MOZ_ASSERT(r == 0);
>   }

If MOZ_ASSERT is expanded to nothing, than the warning is correct. (void)r; 
trick should work here..


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I think there is a false positive with this @george.burgess.iv:
In 
[this)(https://searchfox.org/mozilla-central/source/mozglue/baseprofiler/core/platform-linux-android.cpp#222-227)
 we have the warning triggered: 
mozglue/baseprofiler/core/platform-linux-android.cpp:216:9: error: variable 'r' 
set but not used [-Werror,-Wunused-but-set-variable]

  SigHandlerCoordinator() {
PodZero();
int r = sem_init(, /* pshared */ 0, 0);
r |= sem_init(, /* pshared */ 0, 0);
r |= sem_init(, /* pshared */ 0, 0);
MOZ_ASSERT(r == 0);
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[George Burgess IV via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf49cae278b4: [Clang] -Wunused-but-set-parameter and 
-Wunused-but-set-variable (authored by mbenfield, committed by 
george.burgess.iv).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+struct __attribute__((warn_unused)) SWarnUnused {
+  int j;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  // Unless it's marked with the warn_unused attribute.
+  struct SWarnUnused swu; // expected-warning{{variable 'swu' set but not used}}
+  struct SWarnUnused swu2;
+  swu = swu2;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Also don't warn for a reference.
+void f4(int ) {
+  x = 0;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

Removing from queue - i don't expect to review this.
Looks like this has been reverted twice now, presumably llvm stage 2/linux 
kernel/chrome
should be enough of a coverage to be sure there isn't any obvious 
false-positives this time.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 347111.
mbenfield added a comment.

Move fixes into a separate change https://reviews.llvm.org/D102942.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+struct __attribute__((warn_unused)) SWarnUnused {
+  int j;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  // Unless it's marked with the warn_unused attribute.
+  struct SWarnUnused swu; // expected-warning{{variable 'swu' set but not used}}
+  struct SWarnUnused swu2;
+  swu = swu2;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Also don't warn for a reference.
+void f4(int ) {
+  x = 0;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> Also fix a few places where this warning is correctly triggered.

Create new patch please - dont mix fixes with new warning within one patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 346904.
mbenfield added a comment.
Herald added subscribers: llvm-commits, lldb-commits, kbarton, hiraditya, 
nemanjai.
Herald added projects: LLDB, LLVM.

Don't warn for reference or dependent types (fixing false positives).

Also fix a few places where this warning is correctly triggered. In a couple
places I did that by adding __attribute__((unused)) rather than removing the
unused variable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Plugins/ABI/PowerPC/ABISysV_ppc.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV1.cpp
  llvm/lib/Target/AArch64/GISel/AArch64PostSelectOptimize.cpp
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
  llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
  llvm/lib/Target/X86/X86FloatingPoint.cpp
  llvm/utils/benchmark/src/complexity.cc

Index: llvm/utils/benchmark/src/complexity.cc
===
--- llvm/utils/benchmark/src/complexity.cc
+++ llvm/utils/benchmark/src/complexity.cc
@@ -76,7 +76,6 @@
 LeastSq MinimalLeastSq(const std::vector& n,
const std::vector& time,
BigOFunc* fitting_curve) {
-  double sigma_gn = 0.0;
   double sigma_gn_squared = 0.0;
   double sigma_time = 0.0;
   double sigma_time_gn = 0.0;
@@ -84,7 +83,6 @@
   // Calculate least square fitting parameter
   for (size_t i = 0; i < n.size(); ++i) {
 double gn_i = fitting_curve(n[i]);
-sigma_gn += gn_i;
 sigma_gn_squared += gn_i * gn_i;
 sigma_time += time[i];
 sigma_time_gn += time[i] * gn_i;
Index: llvm/lib/Target/X86/X86FloatingPoint.cpp
===
--- llvm/lib/Target/X86/X86FloatingPoint.cpp
+++ llvm/lib/Target/X86/X86FloatingPoint.cpp
@@ -1526,7 +1526,7 @@
 
 // Scan the assembly for ST registers used, defined and clobbered. We can
 // only tell clobbers from defs by looking at the asm descriptor.
-unsigned STUses = 0, STDefs = 0, STClobbers = 0, STDeadDefs = 0;
+unsigned STUses = 0, STDefs = 0, STClobbers = 0;
 unsigned NumOps = 0;
 SmallSet FRegIdx;
 unsigned RCID;
@@ -1559,8 +1559,6 @@
   case InlineAsm::Kind_RegDef:
   case InlineAsm::Kind_RegDefEarlyClobber:
 STDefs |= (1u << STReg);
-if (MO.isDead())
-  STDeadDefs |= (1u << STReg);
 break;
   case InlineAsm::Kind_Clobber:
 STClobbers |= (1u << STReg);
Index: llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
===
--- llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
+++ llvm/lib/Target/PowerPC/PPCBranchSelector.cpp
@@ -415,5 +415,5 @@
   }
 
   BlockSizes.clear();
-  return true;
+  return EverMadeChange;
 }
Index: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
===
--- llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
+++ llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
@@ -1247,7 +1247,7 @@
   if (LoLoop.Preheader)
 LoLoop.Start = SearchForStart(LoLoop.Preheader);
   else
-return false;
+return Changed;
 
   // Find the low-overhead loop components and decide whether or not to fall
   // back to a normal loop. Also look for a vctp instructions and decide
@@ -1281,7 +1281,7 @@
   LLVM_DEBUG(LoLoop.dump());
   if (!LoLoop.FoundAllComponents()) {
 LLVM_DEBUG(dbgs() << "ARM Loops: Didn't find loop start, update, end\n");
-return false;
+return Changed;
   }
 
   assert(LoLoop.Start->getOpcode() != ARM::t2WhileLoopStart &&
Index: llvm/lib/Target/ARM/ARMISelLowering.cpp
===
--- llvm/lib/Target/ARM/ARMISelLowering.cpp
+++ llvm/lib/Target/ARM/ARMISelLowering.cpp
@@ 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

This warning seems to have a lot of false positives on things like reference 
arguments that are used as output parameters. For example here is a small 
sample of output from a stage2 build of part of LLVM:

  In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
  In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:21:
  In file included from ../llvm/include/llvm/ADT/BitmaskEnum.h:16:
  ../llvm/include/llvm/Support/MathExtras.h:822:9: warning: variable 
'Overflowed' set but not used [-Wunused-but-set-variable]
bool  = ResultOverflowed ? *ResultOverflowed : Dummy;
  ^
  ../llvm/include/llvm/Support/MathExtras.h:936:72: warning: parameter 'Result' 
set but not used [-Wunused-but-set-parameter]
  std::enable_if_t::value, T> MulOverflow(T X, T Y, T 
) {
 ^
  In file included from ../llvm/lib/BinaryFormat/Minidump.cpp:9:
  In file included from ../llvm/include/llvm/BinaryFormat/Minidump.h:22:
  In file included from ../llvm/include/llvm/ADT/DenseMapInfo.h:20:
  ../llvm/include/llvm/ADT/StringRef.h:511:37: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  getAsInteger(unsigned Radix, T ) const {
  ^
  ../llvm/include/llvm/ADT/StringRef.h:522:37: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  getAsInteger(unsigned Radix, T ) const {
  ^
  ../llvm/include/llvm/ADT/StringRef.h:545:39: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  consumeInteger(unsigned Radix, T ) {
^
  ../llvm/include/llvm/ADT/StringRef.h:556:39: warning: parameter 'Result' set 
but not used [-Wunused-but-set-parameter]
  consumeInteger(unsigned Radix, T ) {
^
  6 warnings generated.

Could you please take a look?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG14dfb3831c42: [Clang] -Wunused-but-set-parameter and 
-Wunused-but-set-variable (authored by mbenfield, committed by aeubanks).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+struct __attribute__((warn_unused)) SWarnUnused {
+  int j;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  // Unless it's marked with the warn_unused attribute.
+  struct SWarnUnused swu; // expected-warning{{variable 'swu' set but not used}}
+  struct SWarnUnused swu2;
+  swu = swu2;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks accepted this revision.
aeubanks added a comment.

If you'd like you can split this into separate changes, one for adding the 
warning and another for adding it into warning groups, either way is fine.
Seems like the reported false positives have been addressed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

FWIW `gcc` does not warn in this case regardless of any cast:

  void f() {
int x;
x = 0;
sizeof(x);
  }


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

casting to void is the normal way to get around these warnings, these new 
warnings should also not diagnose anything with a cast to void


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100581#2730743 , @nickdesaulniers 
wrote:

> Testing Diff 342071 on the mainline Linux kernel, just building x86_64 
> defconfig triggers 19 instances of this warning; all look legit. ;)
>
> In D100581#2730708 , @dblaikie 
> wrote:
>
>> In D100581#2730557 , 
>> @nickdesaulniers wrote:
>>
>>> I see lots of instances from the kernel that look like this when reduced:
>>>
>>>   $ cat foo.c
>>>   int page;
>>>   int put_page_testzero(int);
>>>   void foo (void) {
>>> int zeroed;
>>> zeroed = put_page_testzero(page);
>>> ((void)(sizeof(( long)(!zeroed;
>>>   }
>>>   $ clang -c -Wall foo.c
>>>   foo.c:4:7: warning: variable 'zeroed' set but not used 
>>> [-Wunused-but-set-variable]
>>> int zeroed;
>>> ^
>>
>> Any idea what the purpose of this code is/why it's not a reasonable thing to 
>> warn on?
>
> include/linux/build_bug.h:
>
>   25 /*   
>   
>  
>   26  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of 
> the   
>   
>   27  * expression but avoids the generation of any code, even if that 
> expression
> 
>   28  * has side-effects. 
>   
>  
>   29  */  
>   
>  
>   30 #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e
>
> include/linux/mmdebug.h:
>
>   57 #define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond)   
>   
>  
>
> mm/internal.h:
>
>   410 static inline unsigned long 
>   
>   
>   411 vma_address(struct page *page, struct vm_area_struct *vma)  
>   
>   
>   412 {   
>   
>   
>   413   unsigned long start, end; 
>   
>   
>   414 
>   
>   
>   415   start = __vma_address(page, vma); 
>   
>   
>   416   end = start + thp_size(page) - PAGE_SIZE; 
>   
>   
>   417 
>   
>   
>   418   /* page should be within @vma mapping range */
>   
>   
>   419   VM_BUG_ON_VMA(end < vma->vm_start || start >= vma->vm_end, vma);
>
> The warning (in previous revisions was triggering on `end` in `vma_address`.

Ah, fair enough. I guess this means the warning has the same false positive 
with a value that's initialized unconditional, but then read only in an assert 
(& thus unread in a non-asserts build)? As much as we're used to accepting that 
as a limitation of assert, I can see how that'd be good to avoid for new 
warnings to reduce false positives/code that would require modifications 
despite there being no bug in it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D100581#2730760 , @mbenfield wrote:

> In D100581#2730557 , 
> @nickdesaulniers wrote:
>
>> I see lots of instances from the kernel that look like this when reduced:
>
>
>
>> But adding a new warning flags to a handful of existing tests' RUN lines is 
>> unusual.
>
> These tests use `-Wall` for whatever reason, and they assign but don't use 
> several variables, so we'll get warnings on them unless I add 
> `-Wno-unused-but-set-variable`. Other alternatives:
>
> 1. add an expected-warning;
>
> 2. use a pragma to disable the warning;
>
> 3. put an `unused` attribute on the variables in question;
>
> 4. introduce an artificial use of these variables.
>
> I previously used item 1, but somebody earlier in an earlier comment 
> requested that instead I add `-Wno-unused-but-set-variable` instead.

Ah, I missed that they were _disabling_ the warning; I was thinking they were 
_enabling_ it. Sorry for the noise; LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2730557 , @nickdesaulniers 
wrote:

> I see lots of instances from the kernel that look like this when reduced:



> But adding a new warning flags to a handful of existing tests' RUN lines is 
> unusual.

These tests use `-Wall` for whatever reason, and they assign but don't use 
several variables, so we'll get warnings on them unless I add 
`-Wno-unused-but-set-variable`. Other alternatives:

1. add an expected-warning;

2. use a pragma to disable the warning;

3. put an `unused` attribute on the variables in question;

4. introduce an artificial use of these variables.

I previously used item 1, but somebody earlier in an earlier comment requested 
that instead I add `-Wno-unused-but-set-variable` instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Testing Diff 342071 on the mainline Linux kernel, just building x86_64 
defconfig triggers 19 instances of this warning; all look legit. ;)

In D100581#2730708 , @dblaikie wrote:

> In D100581#2730557 , 
> @nickdesaulniers wrote:
>
>> I see lots of instances from the kernel that look like this when reduced:
>>
>>   $ cat foo.c
>>   int page;
>>   int put_page_testzero(int);
>>   void foo (void) {
>> int zeroed;
>> zeroed = put_page_testzero(page);
>> ((void)(sizeof(( long)(!zeroed;
>>   }
>>   $ clang -c -Wall foo.c
>>   foo.c:4:7: warning: variable 'zeroed' set but not used 
>> [-Wunused-but-set-variable]
>> int zeroed;
>> ^
>
> Any idea what the purpose of this code is/why it's not a reasonable thing to 
> warn on?

include/linux/build_bug.h:

  25 /* 

 
  26  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of 
the 

  27  * expression but avoids the generation of any code, even if that 
expression  
  
  28  * has side-effects.   

 
  29  */

 
  30 #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e

include/linux/mmdebug.h:

  57 #define VM_BUG_ON(cond) BUILD_BUG_ON_INVALID(cond) 

 

mm/internal.h:

  410 static inline unsigned long   

  
  411 vma_address(struct page *page, struct vm_area_struct *vma)

  
  412 { 

  
  413   unsigned long start, end;   

  
  414   

  
  415   start = __vma_address(page, vma);   

  
  416   end = start + thp_size(page) - PAGE_SIZE;   

  
  417   

  
  418   /* page should be within @vma mapping range */  

  
  419   VM_BUG_ON_VMA(end < vma->vm_start || start >= vma->vm_end, vma);

The warning (in previous revisions was triggering on `end` in `vma_address`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

The additions of `-Wno-unused-but-set-variable` to:

- clang/test/SemaObjC/foreach.m
- clang/test/SemaCXX/sizeless-1.cpp
- clang/test/SemaCXX/shift.cpp
- clang/test/SemaCXX/goto.cpp
- clang/test/Sema/vector-gcc-compat.cpp
- clang/test/Sema/vector-gcc-compat.c
- clang/test/FixIt/fixit.cpp
- clang/test/CodeGen/builtins-arm.c
- clang/test/CodeGen/builtins-riscv.c
- clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp

...
all the one line changes to tests that add this warning flag to the run line 
look questionable to me; we generally don't add new warning flags to a ton of 
existing tests.  Were those cases that were previously failing? Were those 
accidentally committed?  I would have expected this change to be:

- changes to enable the flag and do something
- block of additional test cases
- changes to existing tests if they were failing this new check inadvertantly.

But adding a new warning flags to a handful of existing tests' RUN lines is 
unusual.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100581#2730557 , @nickdesaulniers 
wrote:

> I see lots of instances from the kernel that look like this when reduced:
>
>   $ cat foo.c
>   int page;
>   int put_page_testzero(int);
>   void foo (void) {
> int zeroed;
> zeroed = put_page_testzero(page);
> ((void)(sizeof(( long)(!zeroed;
>   }
>   $ clang -c -Wall foo.c
>   foo.c:4:7: warning: variable 'zeroed' set but not used 
> [-Wunused-but-set-variable]
> int zeroed;
> ^

Any idea what the purpose of this code is/why it's not a reasonable thing to 
warn on?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 342071.
mbenfield added a comment.

Count a non-ODR use as a reference, so that examples like that of 
nickdesaulniers don't warn.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+struct __attribute__((warn_unused)) SWarnUnused {
+  int j;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  // Unless it's marked with the warn_unused attribute.
+  struct SWarnUnused swu; // expected-warning{{variable 'swu' set but not used}}
+  struct SWarnUnused swu2;
+  swu = swu2;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

I see lots of instances from the kernel that look like this when reduced:

  $ cat foo.c
  int page;
  int put_page_testzero(int);
  void foo (void) {
int zeroed;
zeroed = put_page_testzero(page);
((void)(sizeof(( long)(!zeroed;
  }
  $ clang -c -Wall foo.c
  foo.c:4:7: warning: variable 'zeroed' set but not used 
[-Wunused-but-set-variable]
int zeroed;
^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 342037.
mbenfield added a comment.

Correct struct and vector behavior.

gcc doesn't warn for any structs in C++. However, it _does_ warn for vector 
types. Those warnings are sometimes masked by other errors, leading to my 
earlier belief that it is inconsistent about warning for vector types.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/vector-gcc-compat.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+struct __attribute__((warn_unused)) SWarnUnused {
+  int j;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  // Unless it's marked with the warn_unused attribute.
+  struct SWarnUnused swu; // expected-warning{{variable 'swu' set but not used}}
+  struct SWarnUnused swu2;
+  swu = swu2;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

Apparently arc and/or Phabricator doesn't like my attempt to split this review 
into two separate commits. Next revision will have only one commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticGroups.td:876
 UnusedLocalTypedef, UnusedValue, UnusedVariable,
-UnusedPropertyIvar]>,
+UnusedButSetVariable, UnusedPropertyIvar]>,
 DiagCategory<"Unused Entity Issue">;

llvm-project/clang/include/clang/Basic/DiagnosticGroups.td:874:25: error: 
Variable not defined: 'UnusedButSetVariable'
UnusedButSetVariable, UnusedPropertyIvar]>,
^


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 341965.
mbenfield added a comment.

A second commit with UnusedButSetParameter and UnusedButSetVariable into groups.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/test/CXX/expr/expr.prim/expr.prim.lambda/p12.cpp
  clang/test/CodeGen/2007-10-30-Volatile.c
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
 
 namespace std {
 struct type_info;
Index: clang/test/SemaCXX/shift.cpp
===
--- clang/test/SemaCXX/shift.cpp
+++ clang/test/SemaCXX/shift.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -Wall -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_cc1 -Wall -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify=expected,cxx2a -std=c++2a %s
 
 #include 
 
Index: clang/test/SemaCXX/goto.cpp
===
--- clang/test/SemaCXX/goto.cpp
+++ clang/test/SemaCXX/goto.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wall -fblocks %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wall -Wno-unused-but-set-variable -fblocks %s
 
 // PR9463
 double *end;
Index: clang/test/Sema/vector-gcc-compat.c
===
--- clang/test/Sema/vector-gcc-compat.c
+++ clang/test/Sema/vector-gcc-compat.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -triple x86_64-apple-darwin10
+// RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -Wno-unused-but-set-variable -triple x86_64-apple-darwin10
 
 // Test the compatibility of clang's vector extensions with gcc's vector
 // extensions for C. Notably &&, ||, ?: and ! are not available.
Index: clang/test/Sema/shift.c
===
--- clang/test/Sema/shift.c
+++ clang/test/Sema/shift.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -Wall -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wall -Wno-unused-but-set-variable -Wshift-sign-overflow -ffreestanding -fsyntax-only -verify %s
 
 #include 
 
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100581#2727432 , @mbenfield wrote:

> In D100581#2727357 , @dblaikie 
> wrote:
>
>> 
>
>
>
>> Got a link/examples of cases GCC does and doesn't warn about? I'd assume 
>> it'd have something to do with the triviality or non-triviality of certain 
>> operations of the nonscalar types (eg: is the type trivially 
>> assignable/trivially destructible)
>
> This doesn't seem to be what determines it. AFAICT it never warns for a 
> struct in C++. However, if I do
>
>   gcc -fsyntax-only -Wunused-but-set-variable 
> clang/test/Sema/vector-gcc-compat.c
>
> I get warnings for the variables v2i64_r and v4i32_r.
>
> But if I do
>
>   g++ -fsyntax-only -Wunused-but-set-variable 
> clang/test/Sema/vector-gcc-compat.c
>
> I only get warnings for the variable v2i64_r.
>
> I will investigate a little more tomorrow.

Ah, might have a size threshold?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D100581#2727460 , @aeubanks wrote:

> regarding adding these to a warning group in a separate change, it's easier 
> to revert a smaller second change that adds this to warning groups, and 
> easier to iterate upon in tree when there are discovered issues
> it's not dead code if it has tests

I meant dead code in terms of users. You can always commit this patch as two - 
first with new warning, second - add to groups.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

regarding adding these to a warning group in a separate change, it's easier to 
revert a smaller second change that adds this to warning groups, and easier to 
iterate upon in tree when there are discovered issues
it's not dead code if it has tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

Will also make a new revision tomorrow with these warnings added to the 
appropriate groups.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2727357 , @dblaikie wrote:

> 



> Got a link/examples of cases GCC does and doesn't warn about? I'd assume it'd 
> have something to do with the triviality or non-triviality of certain 
> operations of the nonscalar types (eg: is the type trivially 
> assignable/trivially destructible)

This doesn't seem to be what determines it. AFAICT it never warns for a struct 
in C++. However, if I do

  gcc -fsyntax-only -Wunused-but-set-variable 
clang/test/Sema/vector-gcc-compat.c

I get warnings for the variables v2i64_r and v4i32_r.

But if I do

  g++ -fsyntax-only -Wunused-but-set-variable 
clang/test/Sema/vector-gcc-compat.c

I only get warnings for the variable v2i64_r.

I will investigate a little more tomorrow.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

I'm happy to retest with a few Linux kernel builds...tomorrow!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

>> I've removed these warnings from the diagnostic groups -Wextra

But to remove all false positives, you need reports from users of -Wextra. 
Otherwise this would be dead code with no users.

I suggest to leave it in groups. Now you can ask folks to try this patch with 
kernel, Firefox, Chrome, XYZ and of course LLVM. Also you can try gcc’s 
testcases for this warning. If it is OK, I see no reasons to remove it from 
those groups.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100581#2727330 , @mbenfield wrote:

> Another try at these warnings, using the implementation strategy outlined by 
> rsmith.
>
> A couple other notes:
>
> - At the moment I've removed these warnings from the diagnostic groups 
> -Wextra and -Wunused. It was suggested by aeubanks that the groups could be 
> added in a later commit, once these warnings have been determined to not be 
> too disruptive. Of course I can change this now if requested.
>
> - I've just tried to mimic gcc's behavior as closely as possible, including 
> not warning if an assignment is used, and not warning on nonscalar types in 
> C++ (except that in some cases gcc inexplicably does warn on nonscalar types; 
> test on the file vector-gcc-compat.c to compare... I haven't determined any 
> rationale to when it chooses to warn in these cases).

Got a link/examples of cases GCC does and doesn't warn about? I'd assume it'd 
have something to do with the triviality or non-triviality of certain 
operations of the nonscalar types (eg: is the type trivially 
assignable/trivially destructible)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 341695.
mbenfield added a comment.

Another try at these warnings, using the implementation strategy outlined by 
rsmith.

A couple other notes:

- At the moment I've removed these warnings from the diagnostic groups -Wextra

and -Wunused. It was suggested by aeubanks that the groups could be added in a
later commit, once these warnings have been determined to not be too
disruptive. Of course I can change this now if requested.

- I've just tried to mimic gcc's behavior as closely as possible, including not

warning if an assignment is used, and not warning on nonscalar types in C++
(except that in some cases gcc inexplicably does warn on nonscalar types; test
on the file vector-gcc-compat.c to compare... I haven't determined any
rationale to when it chooses to warn in these cases).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp

Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/Sema/warn-unused-but-set-variables.c
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-variables.c
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  struct S s; // expected-warning{{variable 's' set but not used}}
+  struct S t;
+  s = t;
+
+  // Don't warn for an extern variable.
+  extern int w;
+  w = 0;
+
+  // Following gcc, this should not warn.
+  int a;
+  w = (a = 0);
+
+  int x;
+  x = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
Index: clang/test/Sema/warn-unused-but-set-parameters.c
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-parameters.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+void f3(struct S s) { // expected-warning{{parameter 's' set but not used}}
+  struct S t;
+  s = t;
+}
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2723801 , @xbolva00 wrote:

> 



> Yes, the best solution. Also consider to check gcc’s test-suite - if they 
> have this type of testcase, then this is wanted behaviour.

gcc has `Wunused-var-5.c`, where the warning `-Wunused-but-set-variable` is 
applied but

  void bar (int, ...);
  void f18 (void) { _Atomic int x = 0; int y = 3; bar (x = y); }

is not expected to trigger the warning, which does seem to imply this is 
intentional.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

When this change returns, I'd like to see a different implementation strategy. 
Running a recursive AST visitation after the fact is generally not the right 
way to look for this kind of issue; adding an extra pass to speculatively hunt 
for each kind of warning we might want to issue is too expensive. Instead, I 
think we should do something simpler and cheaper, such as tracking, for each 
variable in scope, the number of potentially-evaluated non-assignment 
references to that variable. For example:

- in `DoMarkVarDeclReferenced`, increment a per-variable counter if the 
variable is a local and the reference is an odr-use
- in `IgnoredValueConversions`, if we're discarding a potentially-evaluated 
assignment or increment expression acting on a local variable, decrement the 
counter for that variable
- in `ActOnPopScope`, or perhaps in `DiagnoseUnusedDecl`, when we're diagnosing 
unused variables, also diagnose set-but-not-used variables (where the variable 
is marked as used but the counter is equal to zero)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D100581#2723794 , @nickdesaulniers 
wrote:

> In D100581#2723789 , @mbenfield 
> wrote:
>
>> But gcc's behavior is to not trigger here so maybe following it is best.
>
> Perhaps file a bug against GCC to find whether that was intentional or a bug?

Yes, the best solution. Also consider to check gcc’s test-suite - if they have 
this type of testcase, then this is wanted behaviour.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D100581#2723789 , @mbenfield wrote:

> But gcc's behavior is to not trigger here so maybe following it is best.

Perhaps file a bug against GCC to find whether that was intentional or a bug?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

So it seems there are 3 issues here:

1. local variables declared extern should not trigger this warning. I'll fix 
this.

2. Should `x` in this code trigger the warning:

  int x;
  int y = (x = 0);

These are essentially the "false positives" reported in the Linux kernel. I 
find it questionable to consider these "false positives"; it seems to me that 
`x` is not really used. But gcc's behavior is to not trigger here so maybe 
following it is best.

3. If `x` above should not trigger the warning, it might be difficult to detect 
this case with an AST walk using the current data. Would it be acceptable to 
add a bit to `VarDecl` or `Decl` indicating whether it's ever //read// (not 
just referenced), which could then be set, maybe in `Sema::ActOnFinishFullExpr` 
? This would then prevent the need for the new AST walk altogether. (it seems 
gcc has such a bit)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D100581#2723709 , @aeubanks wrote:

> reverted
> regarding having a reviewer who is knowledgable in clang diagnostics, I 
> assumed that george.burgess.iv was knowledgable and was happy with the change 
> after his comments, perhaps I should have waited for an explicit LGTM

It's fine, there are always bugs everywhere, won't all be caught.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

reverted
regarding having a reviewer who is knowledgable in clang diagnostics, I assumed 
that george.burgess.iv was knowledgable and was happy with the change after his 
comments, perhaps I should have waited for an explicit LGTM


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D100581#2723621 , @Abpostelnicu 
wrote:

> In D100581#2723611 , @lebedev.ri 
> wrote:
>
>> In D100581#2723505 , @Abpostelnicu 
>> wrote:
>>
>>> Also I don’t remember seeing this proposed on cfe dev mailing list.
>>
>> There is no such requirement. I don't recall that happening basically ever, 
>> actually.
>
> Of course, but I would have wanted to see this discussed on the mailing list 
> since the pool of the affected actors by this is very big.

If we subtract the false-positives, i'm not really sure what should be 
discussed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

In D100581#2723611 , @lebedev.ri 
wrote:

> In D100581#2723505 , @Abpostelnicu 
> wrote:
>
>> Also I don’t remember seeing this proposed on cfe dev mailing list.
>
> There is no such requirement. I don't recall that happening basically ever, 
> actually.

Of course, but I would have wanted to see this discussed on the mailing list 
since the pool of the affected actors by this is very big.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D100581#2723505 , @Abpostelnicu 
wrote:

> Also I don’t remember seeing this proposed on cfe dev mailing list.

There is no such requirement. I don't recall that happening basically ever, 
actually.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

Link for the revert: https://reviews.llvm.org/D101480


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

Please revert this. This change hasn’t been tested nor reviewed enough. Also I 
don’t remember seeing this proposed on cfe dev mailing list.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2723348 , @lebedev.ri 
wrote:

> Also, from the reviewer list, it doesn't look like anyone with much 
> familiarity with clang/clang diags actually participated in the review?
> I think this might be the point to revert and re-review.

Alright, someone with the capability please revert if that's the correct action.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Also, from the reviewer list, it doesn't look like anyone with much familiarity 
with clang/clang diags actually participated in the review?
I think this might be the point to revert and re-review.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D100581#2723329 , @nathanchance 
wrote:

> In D100581#2721430 , 
> @nickdesaulniers wrote:
>
>> Huh, maybe not: https://godbolt.org/z/PnE1fMGWo
>
> This difference has caused some confusion already for the Linux kernel:

It's quite obviously used, since it's the condition variable for the loop.
Is clang CFG not modelling that correctly?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

In D100581#2721430 , @nickdesaulniers 
wrote:

> Huh, maybe not: https://godbolt.org/z/PnE1fMGWo

This difference has caused some confusion already for the Linux kernel: 
https://lore.kernel.org/r/202104280827.lsczw8xg-...@intel.com

I am not sure it is appropriate to warn there.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dmitry Babokin via Phabricator via cfe-commits]

dbabokin added a comment.

> Thanks for the report. It seems the thing to do is to modify the warning so 
> that variables marked extern are not candidates for this warning. I will make 
> a new commit with that modification unless there are other suggestions.

I agree, this seems to be the right fix. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2721705 , @dbabokin wrote:

> One more false-positive:
>
>   void foo() {
> extern int yydebug;
> yydebug = 1;
>   }
>
> It was triggered in ISPC build.

Thanks for the report. It seems the thing to do is to modify the warning so 
that variables marked extern are not candidates for this warning. I will make a 
new commit with that modification unless there are other suggestions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dmitry Babokin via Phabricator via cfe-commits]

dbabokin added a comment.

One more false-positive:

  void foo() {
extern int yydebug;
yydebug = 1;
  }

It was triggered in ISPC build.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Huh, maybe not: https://godbolt.org/z/PnE1fMGWo


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

I think this is triggering some false positives in builds of the Linux kernel.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

Right, our commit process is a bit strange. Ideally, the change author should 
initiate commit after they have the necessary approvals, and then be 
responsible to deal with any fallout in a timely manner. As things are, the 
author does not control the timing of the commit, and the committer does not 
get the bot messages.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Sorry about that, although I don't think emails went to me since the email was 
somebody else's. But point taken, I'll coordinate better in the future when 
landing someone else's patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Evgenii Stepanov via Phabricator via cfe-commits]

eugenis added a comment.

I fixed another one in 
https://reviews.llvm.org/rG561f4b9087457e9ea3cf4aeb57dcd507e2fa6258
Please watch the bots after you land changes like this one and don't let them 
stay broken for an entire day!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D100581#2718947 , @Abpostelnicu 
wrote:

> I think this added a regression, where you have this context:
>
>   uint32_t LocalAccessible::SelectedItemCount() {
> uint32_t count = 0;
> AccIterator iter(this, filters::GetSelected);
> LocalAccessible* selected = nullptr;
> while ((selected = iter.Next())) ++count;
>   
> return count;
>   }
>
>
>
>   [task 2021-04-27T02:39:42.523Z] 02:39:42ERROR -  
> /builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.cpp:2455:20:
>  error: variable 'selected' set but not used 
> [-Werror,-Wunused-but-set-variable]
>   [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO -LocalAccessible* 
> selected = nullptr;
>   [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO - ^
>   [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO -  1 error generated.
>
> The file in cause is this 
> 
>  one.
>
> Indeed the value stored in `selected` is not used but this check is a little 
> bit too strict.

That seems like a real finding to me, deleting `selected` should fix it and 
clean up the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

In D100581#2718840 , @uabelho wrote:

> I noticed that with this patch
>  
> libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp
> fails:
>
>   error: 'warning' diagnostics seen but not expected: 
> File 
> /repo/uabelho/master-github/libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp
>  Line 51: variable 's' set but not used
>   1 error generated.
>
> (I don't know anything about the testcase I just noticed it failed when 
> trying to uplift my downstream repo.)

looks like this was fixed in 7f98209da6349891b7a74eeadace34d38e7aaadc 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Andi via Phabricator via cfe-commits]

Abpostelnicu added a comment.

I think this added a regression, where you have this context:

  uint32_t LocalAccessible::SelectedItemCount() {
uint32_t count = 0;
AccIterator iter(this, filters::GetSelected);
LocalAccessible* selected = nullptr;
while ((selected = iter.Next())) ++count;
  
return count;
  }



  [task 2021-04-27T02:39:42.523Z] 02:39:42ERROR -  
/builds/worker/checkouts/gecko/accessible/generic/LocalAccessible.cpp:2455:20: 
error: variable 'selected' set but not used [-Werror,-Wunused-but-set-variable]
  [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO -LocalAccessible* 
selected = nullptr;
  [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO - ^
  [task 2021-04-27T02:39:42.523Z] 02:39:42 INFO -  1 error generated.

The file in cause is this 

 one.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Mikael Holmén via Phabricator via cfe-commits]

uabelho added a comment.

I noticed that with this patch
 libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp
fails:

  error: 'warning' diagnostics seen but not expected: 
File 
/repo/uabelho/master-github/libcxx/test/std/strings/basic.string/string.cons/iter_alloc_deduction.fail.cpp
 Line 51: variable 's' set but not used
  1 error generated.

(I don't know anything about the testcase I just noticed it failed when trying 
to uplift my downstream repo.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9b0501abc7b5: [Clang] -Wunused-but-set-parameter and 
-Wunused-but-set-variable (authored by mbenfield, committed by aeubanks).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

yeah you have to manually go to "Edit Revision" and update the message.

I'll just amend it locally though


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2717182 , @aeubanks wrote:

> I can land this for you, but could you update the description to be a bit 
> more descriptive? Explaining what the warnings do.

Are you looking at the updated message in the commit, or the old summary 
displayed in Phabricator? It seems Phabricator doesn't change the summary when 
I update the commit description. Let me know whether this is what you thought, 
or if the current commit message is not adequate. The current commit message is

[Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

These are intended to mimic warnings available in gcc.

-Wunused-but-set-variable is triggered in the case of a variable which
appears on the LHS of an assignment but not otherwise used.

For instance:

  void f() {
  int x;
  x = 0;
  }

-Wunused-but-set-parameter works similarly, but for function parameters
instead of variables.

In C++, they are triggered only for scalar types; otherwise, they are
triggered for all types. This is gcc's behavior.

-Wunused-but-set-parameter is controlled by -Wextra, while
-Wunused-but-set-variable is controlled by -Wunused. This is slightly
different from gcc's behavior, but seems most consistent with clang's
behavior for -Wunused-parameter and -Wunused-variable.

Differential Revision: https://reviews.llvm.org/D100581


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

I can land this for you, but could you update the description to be a bit more 
descriptive? Explaining what the warnings do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

Great. If it's been approved, I am not able to commit, so I think someone else 
will have to do that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

I think this is good to go. Later anybody can build improvements on top of this 
patch and maybe work on some more powerful “dead stores” warnings.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Looks good in terms of false positives on Chrome. This did uncover a lot of 
issues, although many of them are due to only using some variable in some 
config with `#ifdef`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 339778.
mbenfield added a comment.

Fix test warning-wall.c.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
 
 namespace std {
 struct type_info;
Index: clang/test/SemaCXX/shift.cpp

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield marked 4 inline comments as done.
mbenfield added a comment.

In D100581#2705780 , @xbolva00 wrote:

> Meanwhile I collected some interesting testcases from gcc bugzilla (reported 
> as false positives), please try:
> https://godbolt.org/z/747ndKEvd - No "unused-but-set" warnings expected.

FWIW, neither of these "unused-but-set" warnings were triggered on any of that 
code (although some other warnings were).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D100581#2706554 , @xbolva00 wrote:

> In D100581#2706467 , @dblaikie 
> wrote:
>
>> FWIW, I'd love it if we could do a full dead-store warning, which would be a 
>> superset of this. I think we have enough infrastructure in the analysis 
>> based warnings (I think the sufficiency of the infrastructure is 
>> demonstrated by the "may be used uninitialized" warnings). Such a warning 
>> would subsume these narrower "set but not used" type of warnings (though 
>> would require the analysis warning infrastructure).
>
>
>
> - Compile time cost could be a problem.

Given that we managed to enable -Wsometimes-uninitialized, which is a 
CFG-sensitive warning under -Wall:

  $ clang++-tot maybe-uninit.cpp -Wall -c
  maybe-uninit.cpp:4:7: warning: variable 'i' is used uninitialized whenever 
'if' condition is false [-Wsometimes-uninitialized]
if (b)
^
  maybe-uninit.cpp:6:6: note: uninitialized use occurs here
f1(i);
   ^
  maybe-uninit.cpp:4:3: note: remove the 'if' if its condition is always true
if (b)
^~
  maybe-uninit.cpp:3:8: note: initialize the variable 'i' to silence this 
warning
int i;
 ^
  = 0
  1 warning generated.

Then I think similar infrastructure could be used to implement a dead-store 
warning too.

> - Do we need stronger dead store warning? Clang analyzer checks for dead 
> stores, no?

Catching something at compile time's a fair bit of increased value over using 
the static analyzer (at least at Google we don't use the static analyzer, for 
instance)

Oh, right - https://bugs.llvm.org/show_bug.cgi?id=24506 documents one of the 
cases that came up that I think a dead store warning would be rather helpful at 
catching. (though I think we probably now catch the int case 
-Wzero-as-null-pointer-constant - though that's not on by default, for instance 
because it's too pervasive, but a dead store warning would catch the buggy case 
without require a stylistic change to a whole codebase - and also a dead store 
warning could catch the case with, say a T** parameter where you mean to set 
the caller's value to nullptr, but instead set the local copy to nullptr) - in 
these simple examples unused-but-set catches them, but it's not uncommon to 
test a parameter for non-null before writing, for instance you might mean "if 
(x) *x = nullptr" but write "if (x) x = nullptr" - dead store would catch that 
but unused-but-set would not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D100581#2706467 , @dblaikie wrote:

> FWIW, I'd love it if we could do a full dead-store warning, which would be a 
> superset of this. I think we have enough infrastructure in the analysis based 
> warnings (I think the sufficiency of the infrastructure is demonstrated by 
> the "may be used uninitialized" warnings). Such a warning would subsume these 
> narrower "set but not used" type of warnings (though would require the 
> analysis warning infrastructure).



- Compile time cost could be a problem.
- Do we need stronger dead store warning? Clang analyzer checks for dead 
stores, no?

I would rather have this on by default with -Wall, than something stronger off 
by default


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

FWIW, I'd love it if we could do a full dead-store warning, which would be a 
superset of this. I think we have enough infrastructure in the analysis based 
warnings (I think the sufficiency of the infrastructure is demonstrated by the 
"may be used uninitialized" warnings). Such a warning would subsume these 
narrower "set but not used" type of warnings (though would require the analysis 
warning infrastructure).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Thanks, looks really good.

Meanwhile I collected some interesting testcases from gcc bugzilla (reported as 
false positives), please try:
https://godbolt.org/z/747ndKEvd - No "unused-but-set" warnings expected.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2700083 , @xbolva00 wrote:

> So I think we should put UnusedButSetParameter under -Wextra
>
> So I would just put UnusedButSetVariable under -Wunused (and -Wunused is part 
> of -Wall):
>
> WDYT?

Sounds good; I've done this now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 339275.
mbenfield added a comment.
Herald added subscribers: frasercrmck, luismarques, apazos, sameer.abuasal, 
pengfei, s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, 
rogfer01, edward-jones, zzheng, jrtc27, niosHD, sabuasal, simoncook, johnrusso, 
rbar, asb.

Response to review comments.

- Diagnostic groups: UnusedButSetVariable into Unused and UnusedButSetParameter 
into Extra

- Early exit from VisitBinaryOperator

- Idiomatic `if (Decl *D = ...)`

- Add `-Wno-unused-but-set-parameter` and `-Wno-unused-but-set-variable` to 
many tests (necessary since these were added to diagnostic groups)

- Don't flag for const or constexpr variables

- Test case for const and constexpr variables

- Move C++ test cases to SemaCXX


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CodeGen/X86/x86_32-xsave.c
  clang/test/CodeGen/X86/x86_64-xsave.c
  clang/test/CodeGen/builtins-arm.c
  clang/test/CodeGen/builtins-riscv.c
  clang/test/FixIt/fixit.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/shift.c
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables.c
  clang/test/SemaCXX/goto.cpp
  clang/test/SemaCXX/shift.cpp
  clang/test/SemaCXX/sizeless-1.cpp
  clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
  clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
  clang/test/SemaObjC/foreach.m

Index: clang/test/SemaObjC/foreach.m
===
--- clang/test/SemaObjC/foreach.m
+++ clang/test/SemaObjC/foreach.m
@@ -1,4 +1,4 @@
-/* RUN: %clang_cc1 -Wall -fsyntax-only -verify -std=c89 -pedantic %s
+/* RUN: %clang_cc1 -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -fsyntax-only -verify -std=c89 -pedantic %s
  */
 
 @class NSArray;
Index: clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
+
+void f2() {
+  // Don't warn for either of these cases.
+  constexpr int x = 2;
+  const int y = 1;
+  char a[x];
+  char b[y];
+}
Index: clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/SemaCXX/sizeless-1.cpp
===
--- clang/test/SemaCXX/sizeless-1.cpp
+++ clang/test/SemaCXX/sizeless-1.cpp
@@ -1,7 +1,7 @@
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++11 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++17 %s
-// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=gnu++17 %s
+// RUN: %clang_cc1 -fcxx-exceptions -fsyntax-only -verify -W -Wall -Wno-unused-but-set-parameter -Wno-unused-but-set-variable -Wrange-loop-analysis -triple arm64-linux-gnu -target-feature +sve -std=c++98 %s
+// RUN: 

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

So I checked gcc on godbolt since gcc docs are not so clear.

UnusedButSetParameter is ignored with:

- -Wunused alone
- -Wextra alone
- -Wall -Wunused

But works with -Wextra -Wunused or  -Wall -Wextra.

So I think we should put UnusedButSetParameter under -Wextra

  def Extra : DiagGroup<"extra", [
  DeprecatedCopy,
  MissingFieldInitializers,
  IgnoredQualifiers,
  InitializerOverrides,
  SemiBeforeMethodBody,
  MissingMethodReturnType,
  SignCompare,
  UnusedButSetParameter
  UnusedParameter,
  NullPointerArithmetic,
  EmptyInitStatement,
  StringConcatation,
  FUseLdPath,
]>;

UnusedButSetVariable is part of -Wall and -Wunused (gcc's -Wunused is enabled 
by -Wall.)

So I would just put UnusedButSetVariable under -Wunused (and -Wunused is part 
of -Wall):

  def Unused : DiagGroup<"unused",
 [UnusedArgument, UnusedButSetVariable,
  UnusedFunction, UnusedLabel,
  // UnusedParameter, (matches GCC's behavior)
  // UnusedTemplate, (clean-up libc++ before enabling)
  // UnusedMemberFunction, (clean-up llvm before 
enabling)
  UnusedPrivateField, UnusedLambdaCapture,
  UnusedLocalTypedef, UnusedValue, UnusedVariable,
  UnusedPropertyIvar]>,
  DiagCategory<"Unused Entity Issue">;

WDYT?




Comment at: clang/test/Sema/warn-unused-but-set-variables-cpp.cpp:1
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+

I would suggest move it to clang/test/SemaCXX/warn-unused-but-set-variables.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2699412 , @xbolva00 wrote:

> I am a little bit worried that another off by default warning is not ideal 
> from user point of view. Either the user simply would fail to find out that 
> there is a new option or will be surprised that gcc fires and clang does not 
> even when we claim we implemented this “gcc’s” warning.

Understood. How about putting `-Wunused-but-set-parameter` in `-Wextra` (which 
is what clang does for `-Wunused-parameter`) and `-Wunused-but-set-variable` in 
`-Wunused` (which is what gcc does)? I don't know why the parameter ones go in 
`-Wextra` and the variable ones go in `-Wunused`, but this seems most 
consistent with currrent diagnostic groups.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

another false positive:

  
../../third_party/abseil-cpp/absl/strings/internal/str_format/float_conversion.cc:879:20:
 error: variable 'kBufSizeForHexFloatRepr' set but not used 
[-Werror,-Wunused-but-set-variable]
constexpr size_t kBufSizeForHexFloatRepr =

https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl/strings/internal/str_format/float_conversion.cc;drc=dcb2703f69f9d66a848106b97f6c932a1d02f84e;l=879


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a subscriber: rsmith.
xbolva00 added a comment.

In D100581#2697697 , @mbenfield wrote:

> In D100581#2693131 , @xbolva00 
> wrote:
>
 These warnings are not enabled by any other flags. This is different from 
 gcc, where -Wunused-but-set-variable is enabled by -Wextra in combination 
 with either -Wunused or -Wall.
>>
>> IMHO we should follow gcc here.
>
> I'd be happy to do so, but there are two issues:
>
> 1. I'm not sure this is feasible and fits in with how Clang's diagnostics are 
> organized. AFAICT clang's diagnostics are not set up to have a diagnostic 
> enabled only if //two// other flags are set. If I'm wrong please let me know.
>
> 2. In gcc, this is how `-Wunused-parameter` behaves, but clang's 
> `-Wunused-parameter` is already different. In clang, it's enabled by 
> `-Wextra` regardless of `-Wall` or `-Wunused`.

I am a little bit worried that another off by default warning is not ideal from 
user point of view. Either the user simply would fail to find out that there is 
a new option or will be surprised that gcc fires and clang does not even when 
we claim we implemented this “gcc’s” warning.

About your points, @rsmith may help you.

-


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a reviewer: rtrieu.
george.burgess.iv added a comment.

Just a few more nits and LGTM. We probably want the thoughts of someone with 
ownership in warnings to be sure. +rtrieu might be good?




Comment at: clang/lib/Sema/SemaDecl.cpp:13740
+// other than assigning to it, sets the corresponding value to false.
+static void AreAllUsesSets(Stmt *Body,
+   llvm::SmallDenseMap *Map) {

mbenfield wrote:
> george.burgess.iv wrote:
> > nit: Should this be a `const Stmt*`? I don't think we should be mutating 
> > the `Body`
> Unfortunately the `RecursiveASTVisitor`'s non-overridden member functions 
> don't take `const`.
Yeah, I was thinking of StmtVisitor's interface -- my mistake



Comment at: clang/lib/Sema/SemaDecl.cpp:13813-13818
+// check for Ignored here, because if we have no candidates we can avoid
+// walking the AST
+if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_parameter,
+ P->getLocation()) ==
+DiagnosticsEngine::Ignored)
+  return false;

mbenfield wrote:
> mbenfield wrote:
> > george.burgess.iv wrote:
> > > If we want to optimize for when this warning is disabled, would it be 
> > > better to hoist this to the start of DiagnoseUnusedButSetParameters?
> > Isn't it essentially at the beginning of the function as-is? If the warning 
> > is disabled for all parameters, it'll return false right away for all of 
> > them. However, I will add an extra check to end as soon as possible once no 
> > candidates are found. Let me know if it isn't ideal.
> I didn't modify this. I'm not sure there would be any advantage to checking 
> if warnings are disabled for every parameter before just doing all the checks 
> for all of them. Please push back if you think otherwise. 
Ah, I was misreading a bit; didn't realize that we were using the `SourceLoc` 
of each individual parameter to feed into `getDiagnosticLevel`. Yeah, this is 
fine as-is.



Comment at: clang/lib/Sema/SemaDecl.cpp:13752
+  // This is not an assignment to one of our NamedDecls.
+  TraverseStmt(LHS);
+}

since we try to exit early, should this be

```
if (!TraverseStmt(LHS))
  return false;
```

(and similar below)?



Comment at: clang/lib/Sema/SemaDecl.cpp:13760
+// If we remove all Decls, no need to keep searching.
+return (!S.erase(DRE->getFoundDecl()) || S.size());
+  }

nit: LLVM doesn't like superfluous parens; please write this as `return 
!S.erase(DRE->getFoundDecl()) || S.size();`



Comment at: clang/lib/Sema/SemaDecl.cpp:13825
+  Decl *D = Decl::castFromDeclContext((*Parameters.begin())->getDeclContext());
+  if (D)
+DiagnoseUnusedButSetDecls(this, D, Candidates,

nit: LLVM generally tries to write

```
auto *Foo = bar();
if (Foo)
  use(Foo);
```

as

```
if (auto *Foo = bar())
  use(Foo)
```

in part because the latter makes a bit better use of scopes



Comment at: clang/test/Sema/vector-gcc-compat.c:1
 // RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -triple 
x86_64-apple-darwin10
 

nit: is it possible to add -Wno-unused-but-set-parameter & 
-Wno-unused-but-set-variable as a part of the RUN line?

if it becomes too long, you can use \ to wrap it:

```
// RUN: foo bar \
// RUN: baz
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 338423.
mbenfield added a comment.

Fixed misfirings reported by aeubanks and added tests for those cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/warn-unused-but-set-parameters-cpp.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables-cpp.cpp
  clang/test/Sema/warn-unused-but-set-variables.c

Index: clang/test/Sema/warn-unused-but-set-variables.c
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-variables.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  struct S s; // expected-warning{{variable 's' set but not used}}
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
Index: clang/test/Sema/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // In C++, don't warn for structs. (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x + 5;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
Index: clang/test/Sema/warn-unused-but-set-parameters.c
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-parameters.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+void f3(struct S s) { // expected-warning{{parameter 's' set but not used}}
+  struct S t;
+  s = t;
+}
Index: clang/test/Sema/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// In C++, don't warn for a struct (following gcc).
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
+
+// Make sure this doesn't warn.
+struct A {
+  int i;
+  A(int j) : i(j) {}
+};
Index: clang/test/Sema/vector-gcc-compat.c
===
--- clang/test/Sema/vector-gcc-compat.c
+++ clang/test/Sema/vector-gcc-compat.c
@@ -1,5 +1,8 @@
 // RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -triple x86_64-apple-darwin10
 
+#pragma clang diagnostic ignored "-Wunused-but-set-parameter"
+#pragma clang diagnostic ignored "-Wunused-but-set-variable"
+
 // Test the compatibility of clang's vector extensions with gcc's vector
 // extensions for C. Notably &&, ||, ?: and ! are not available.
 typedef long long v2i64 __attribute__((vector_size(16)));
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -434,7 +434,9 @@
   DiagnoseEmptyLoopBody(Elts[i], Elts[i + 1]);
   }
 
-  return CompoundStmt::Create(Context, Elts, L, R);
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;
 }
 
 ExprResult
Index: clang/lib/Sema/SemaExpr.cpp

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added a comment.

In D100581#2693131 , @xbolva00 wrote:

>>> These warnings are not enabled by any other flags. This is different from 
>>> gcc, where -Wunused-but-set-variable is enabled by -Wextra in combination 
>>> with either -Wunused or -Wall.
>
> IMHO we should follow gcc here.

I'd be happy to do so, but there are two issues:

1. I'm not sure this is feasible and fits in with how Clang's diagnostics are 
organized. AFAICT clang's diagnostics are not set up to have a diagnostic 
enabled only if //two// other flags are set. If I'm wrong please let me know.

2. In gcc, this is how `-Wunused-parameter` behaves, but clang's 
`-Wunused-parameter` is already different. In clang, it's enabled by `-Wextra` 
regardless of `-Wall` or `-Wunused`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;

mbenfield wrote:
> mbenfield wrote:
> > mbenfield wrote:
> > > george.burgess.iv wrote:
> > > > It seems like these two warnings are doing analyses with identical 
> > > > requirements (e.g., the function's body must be present), but are 
> > > > taking two different sets of variables into account while doing so.
> > > > 
> > > > Is there a way we can rephrase this so we only need to walk the AST 
> > > > checking once for all of this?
> > > I'll think about this. It might be tricky. 
> > I'll implement this. It will be a fairly different approach though. 
> I have this implemented. I am going to run a performance comparison on the 
> two approaches before making a new revision here. 
An informal comparison reveals no significant performance difference between 
these two approaches. I think it's a tradeoff: if you walk each block and/or 
each function separately, you do have to do more walks, but you're likely to be 
able to terminate your walk early (because you already ran into a use of each 
variable/parameter).

OTOH, if you walk everything in one go, it is only one walk, but you definitely 
have to walk the whole thing (because you might run into another CompoundStmt 
with more declarations). 

I find the current approach conceptually simpler and so would prefer to keep it 
as-is. If you disagree let me know. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

-Wunused-but-set-variable is firing on `x` at [1]

[1] 
https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl/synchronization/mutex.h;drc=07cb7b184515ad207d30f00d0b00b8ce96d0a750;l=947

  ../../third_party/abseil-cpp/absl/synchronization/mutex.h:947:6: error: 
variable 'x' set but not used [-Werror,-Wunused-but-set-variable]
T *x = static_cast(c->arg_);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added a comment.

Running this over Chromium, I'm getting

  $ cat /tmp/a.cc
  struct A {
int i;
A(int i): i(i) {}
  };
  
  $ ./build/rel/bin/clang++ -fsyntax-only /tmp/a.cc -Wunused-but-set-parameter
  /tmp/a.cc:3:9: warning: parameter 'i' set but not used 
[-Wunused-but-set-parameter]
A(int i): i(i) {}
  ^
  1 warning generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield added inline comments.



Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;

mbenfield wrote:
> mbenfield wrote:
> > george.burgess.iv wrote:
> > > It seems like these two warnings are doing analyses with identical 
> > > requirements (e.g., the function's body must be present), but are taking 
> > > two different sets of variables into account while doing so.
> > > 
> > > Is there a way we can rephrase this so we only need to walk the AST 
> > > checking once for all of this?
> > I'll think about this. It might be tricky. 
> I'll implement this. It will be a fairly different approach though. 
I have this implemented. I am going to run a performance comparison on the two 
approaches before making a new revision here. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield marked 4 inline comments as done.
mbenfield added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:13751
+bool TraverseBinaryOperator(BinaryOperator *BO) {
+  auto *LHS = BO->getLHS();
+  auto *DRE = dyn_cast(LHS);

george.burgess.iv wrote:
> nit: `const auto *` is preferred when possible
Not possible here. TraverseStmt doesn't take a const parameter. 



Comment at: clang/lib/Sema/SemaDecl.cpp:13813-13818
+// check for Ignored here, because if we have no candidates we can avoid
+// walking the AST
+if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_parameter,
+ P->getLocation()) ==
+DiagnosticsEngine::Ignored)
+  return false;

george.burgess.iv wrote:
> If we want to optimize for when this warning is disabled, would it be better 
> to hoist this to the start of DiagnoseUnusedButSetParameters?
Isn't it essentially at the beginning of the function as-is? If the warning is 
disabled for all parameters, it'll return false right away for all of them. 
However, I will add an extra check to end as soon as possible once no 
candidates are found. Let me know if it isn't ideal.



Comment at: clang/lib/Sema/SemaDecl.cpp:13813-13818
+// check for Ignored here, because if we have no candidates we can avoid
+// walking the AST
+if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_parameter,
+ P->getLocation()) ==
+DiagnosticsEngine::Ignored)
+  return false;

mbenfield wrote:
> george.burgess.iv wrote:
> > If we want to optimize for when this warning is disabled, would it be 
> > better to hoist this to the start of DiagnoseUnusedButSetParameters?
> Isn't it essentially at the beginning of the function as-is? If the warning 
> is disabled for all parameters, it'll return false right away for all of 
> them. However, I will add an extra check to end as soon as possible once no 
> candidates are found. Let me know if it isn't ideal.
I didn't modify this. I'm not sure there would be any advantage to checking if 
warnings are disabled for every parameter before just doing all the checks for 
all of them. Please push back if you think otherwise. 



Comment at: clang/lib/Sema/SemaDecl.cpp:13850-13853
+// check for Ignored here, because if we have no candidates we can avoid
+// walking the AST
+if (Diags.getDiagnosticLevel(diag::warn_unused_but_set_variable,
+ VD->getLocation()) ==

george.burgess.iv wrote:
> Same "why not put this at the beginning of `DiagnoseUnusedButSetVariables`?" 
> comment
Ditto. 



Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;

george.burgess.iv wrote:
> It seems like these two warnings are doing analyses with identical 
> requirements (e.g., the function's body must be present), but are taking two 
> different sets of variables into account while doing so.
> 
> Is there a way we can rephrase this so we only need to walk the AST checking 
> once for all of this?
I'll think about this. It might be tricky. 



Comment at: clang/lib/Sema/SemaStmt.cpp:437
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;

mbenfield wrote:
> george.burgess.iv wrote:
> > It seems like these two warnings are doing analyses with identical 
> > requirements (e.g., the function's body must be present), but are taking 
> > two different sets of variables into account while doing so.
> > 
> > Is there a way we can rephrase this so we only need to walk the AST 
> > checking once for all of this?
> I'll think about this. It might be tricky. 
I'll implement this. It will be a fairly different approach though. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

[Michael Benfield via Phabricator via cfe-commits]

mbenfield updated this revision to Diff 337999.
mbenfield marked an inline comment as done.
mbenfield added a comment.

Capitalization and periods for comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100581/new/

https://reviews.llvm.org/D100581

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/lib/Sema/SemaStmt.cpp
  clang/test/Sema/vector-gcc-compat.c
  clang/test/Sema/warn-unused-but-set-parameters-cpp.cpp
  clang/test/Sema/warn-unused-but-set-parameters.c
  clang/test/Sema/warn-unused-but-set-variables-cpp.cpp
  clang/test/Sema/warn-unused-but-set-variables.c

Index: clang/test/Sema/warn-unused-but-set-variables.c
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-variables.c
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  struct S s; // expected-warning{{variable 's' set but not used}}
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
Index: clang/test/Sema/warn-unused-but-set-variables-cpp.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-variables-cpp.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-variable -verify %s
+
+struct S {
+  int i;
+};
+
+int f0() {
+  int y; // expected-warning{{variable 'y' set but not used}}
+  y = 0;
+
+  int z __attribute__((unused));
+  z = 0;
+
+  // no warning for structs in C++ (following gcc's behavior)
+  struct S s;
+  struct S t;
+  s = t;
+
+  int x;
+  x = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^() {
+int y; // expected-warning{{variable 'y' set but not used}}
+y = 0;
+
+int x;
+x = 0;
+return x;
+  };
+}
Index: clang/test/Sema/warn-unused-but-set-parameters.c
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-parameters.c
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+void f3(struct S s) { // expected-warning{{parameter 's' set but not used}}
+  struct S t;
+  s = t;
+}
Index: clang/test/Sema/warn-unused-but-set-parameters-cpp.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-unused-but-set-parameters-cpp.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fblocks -fsyntax-only -Wunused-but-set-parameter -verify %s
+
+int f0(int x,
+   int y, // expected-warning{{parameter 'y' set but not used}}
+   int z __attribute__((unused))) {
+  y = 0;
+  return x;
+}
+
+void f1(void) {
+  (void)^(int x,
+  int y, // expected-warning{{parameter 'y' set but not used}}
+  int z __attribute__((unused))) {
+y = 0;
+return x;
+  };
+}
+
+struct S {
+  int i;
+};
+
+// in C++, don't warn for a struct (following gcc)
+void f3(struct S s) {
+  struct S t;
+  s = t;
+}
Index: clang/test/Sema/vector-gcc-compat.c
===
--- clang/test/Sema/vector-gcc-compat.c
+++ clang/test/Sema/vector-gcc-compat.c
@@ -1,5 +1,8 @@
 // RUN: %clang_cc1 %s -verify -fsyntax-only -Weverything -triple x86_64-apple-darwin10
 
+#pragma clang diagnostic ignored "-Wunused-but-set-parameter"
+#pragma clang diagnostic ignored "-Wunused-but-set-variable"
+
 // Test the compatibility of clang's vector extensions with gcc's vector
 // extensions for C. Notably &&, ||, ?: and ! are not available.
 typedef long long v2i64 __attribute__((vector_size(16)));
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -433,7 +433,9 @@
   DiagnoseEmptyLoopBody(Elts[i], Elts[i + 1]);
   }
 
-  return CompoundStmt::Create(Context, Elts, L, R);
+  CompoundStmt *CS = CompoundStmt::Create(Context, Elts, L, R);
+  DiagnoseUnusedButSetVariables(CS);
+  return CS;
 }
 
 ExprResult
Index: clang/lib/Sema/SemaExpr.cpp
===
---