[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

Thank you for the feedback and the revert!

Distinguishing condition variables from other unused variables in Wunused 
warnings was something I was trying initially, and I agree that it's the best 
way forward.
The difficulty is in how to know whether an unused `VarDecl` is a condition 
variable or not in the phase of emitting `Wunused-variable`. We may need to add 
a boolean flag `IsConditionVar` in `VarDecl` or something.

The reason `[[maybe_unused]]` works well is that it is explicitly handled in 
`Sema::ShouldDiagnoseUnusedDecl` 
(https://github.com/llvm/llvm-project/blob/e7bd43675753476e97e63aa13c13b3498407ed1c/clang/lib/Sema/SemaDecl.cpp#L2005-L2007)

The `used` attribute regression was caused by the "used but not-referenced 
variables should be diagnosed as unused" change. Once we have some way of 
knowing that a `VarDecl` is a condition variable, we should only apply this 
change to condition variables, thereby avoiding regression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D152495#4631829 , @nickdesaulniers 
wrote:

> Here's a more blatant regression caused by this patch.
>
> https://godbolt.org/z/q19Ge64G3
>
> Causing breakage for the Linux kernel:
> https://github.com/ClangBuiltLinux/linux/issues/1926

Because of the amount of disruption and the fact that we're trying to get 17.x 
out the door while transitioning to a new code review workflow, let's revert 
this for now. The `__attribute__((used))` problem seems to be something deeper, 
as `[[maybe_unused]]` does work properly (https://godbolt.org/z/e7YK1G69e), and 
putting this diagnostic under its own warning group will help folks adjust to 
the new behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

Here's a more blatant regression caused by this patch.

https://godbolt.org/z/q19Ge64G3


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Pranav Kant via Phabricator via cfe-commits]

pranavk added a comment.

I agree -Wunused-condition-variable sounds like a good idea. There are still 
numerous instances of this warning/error showing up when doing a self-build of 
LLVM, let alone warnings/errors that are showing up in internal code bases who 
are using LLVM HEAD for compiling their codebases.  
`-Wunused-condition-variable` would make this transition smoother.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D152495#4628903 , @smeenai wrote:

> In D152495#4628877 , @hans wrote:
>
>> In D152495#4628870 , @goncharov 
>> wrote:
>>
>>> due to this change we have a enourmous number of new warnings, on the other 
>>> hand -Wunused-variable is a valuable warning. I am not sure what is the 
>>> policy and best practices for warnings but maybe there is a possiblity to 
>>> make a transition period for enabling this type of warning separetely and 
>>> to allow updating existing code?
>>
>> The usual policy is to put new warnings behind new flags so users can 
>> disable them selectively. It gets trickier when it's existing warnings 
>> getting enhanced like this. Would it be possible to put this new 
>> functionality behind a flag?
>
> Yeah, this has been a recurring issue when existing warnings get enhanced. 
> It's often not feasible to fix all new instances right away, but you don't 
> want to disable or `-Wno-error` the warning either cos then new issues can 
> start creeping in, leaving you with no good options.

(I haven't really read the technical side of this patch yet.)

Yes, from me observing how we internally treat such warnings.

> Maybe we should have a policy around even enhancements to existing warnings 
> always going in their own subgroup, so that we can disable them selectively 
> while not regressing anything? @aaron.ballman, what are your thoughts?

Sometimes an improved diagnostic just triggers in very few new places that can 
be easily handled.
Sometimes there are many more places so that it is difficult to fix in a 
satisfactory timeline (as a rolling update user doesn't want to postpone 
releases a long time).

I think sometimes it's just difficult for contributors to figure out whether a 
diagnostic is going to require substantial cleanup or just a little effort. In 
this case rolling update users (like we) providing feedback is useful whether a 
new sub `-Wxxx` is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D152495#4628887 , @aaron.ballman 
wrote:

> That might not be a bad idea in this case -- perhaps 
> `-Wunused-condition-variable` and group it under `-Wunused-variable`?

Sounds god to me conceptually. (I don't know the code, so don't know how hard 
it is practically.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D152495#4628877 , @hans wrote:

> In D152495#4628870 , @goncharov 
> wrote:
>
>> due to this change we have a enourmous number of new warnings, on the other 
>> hand -Wunused-variable is a valuable warning. I am not sure what is the 
>> policy and best practices for warnings but maybe there is a possiblity to 
>> make a transition period for enabling this type of warning separetely and to 
>> allow updating existing code?
>
> The usual policy is to put new warnings behind new flags so users can disable 
> them selectively. It gets trickier when it's existing warnings getting 
> enhanced like this. Would it be possible to put this new functionality behind 
> a flag?

Yeah, this has been a recurring issue when existing warnings get enhanced. It's 
often not feasible to fix all new instances right away, but you don't want to 
disable or `-Wno-error` the warning either cos then new issues can start 
creeping in, leaving you with no good options. Maybe we should have a policy 
around even enhancements to existing warnings always going in their own 
subgroup, so that we can disable them selectively while not regressing 
anything? @aaron.ballman, what are your thoughts?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D152495#4628877 , @hans wrote:

> In D152495#4628785 , @cor3ntin 
> wrote:
>
>> It is used, but only in an assert. Seems like the right fix!
>
> I suppose it is technically, but I'm not sure the fix reads like an 
> improvement to me... I guess that's often the case with unused variables vs. 
> asserts though.
>
> In D152495#4628870 , @goncharov 
> wrote:
>
>> due to this change we have a enourmous number of new warnings, on the other 
>> hand -Wunused-variable is a valuable warning. I am not sure what is the 
>> policy and best practices for warnings but maybe there is a possiblity to 
>> make a transition period for enabling this type of warning separetely and to 
>> allow updating existing code?
>
> The usual policy is to put new warnings behind new flags so users can disable 
> them selectively. It gets trickier when it's existing warnings getting 
> enhanced like this. Would it be possible to put this new functionality behind 
> a flag?

That might not be a bad idea in this case -- perhaps 
`-Wunused-condition-variable` and group it under `-Wunused-variable`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In D152495#4628785 , @cor3ntin wrote:

> It is used, but only in an assert. Seems like the right fix!

I suppose it is technically, but I'm not sure the fix reads like an improvement 
to me... I guess that's often the case with unused variables vs. asserts though.

In D152495#4628870 , @goncharov wrote:

> due to this change we have a enourmous number of new warnings, on the other 
> hand -Wunused-variable is a valuable warning. I am not sure what is the 
> policy and best practices for warnings but maybe there is a possiblity to 
> make a transition period for enabling this type of warning separetely and to 
> allow updating existing code?

The usual policy is to put new warnings behind new flags so users can disable 
them selectively. It gets trickier when it's existing warnings getting enhanced 
like this. Would it be possible to put this new functionality behind a flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Mikhail Goncharov via Phabricator via cfe-commits]

goncharov added a comment.

due to this change we have a enourmous number of new warnings, on the other 
hand -Wunused-variable is a valuable warning. I am not sure what is the policy 
and best practices for warnings but maybe there is a possiblity to make a 
transition period for enabling this type of warning separetely and to allow 
updating existing code?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

It is used, but only in an assert. Seems like the right fix!

In D152495#4628778 , @smeenai wrote:

> In D152495#4628028 , @goncharov 
> wrote:
>
>> there is a number of unused vaiables in conditional loops that are firing 
>> now, I have submitted  
>> https://github.com/llvm/llvm-project/commit/74f4daef0412be33002bd4e24a30cb47d0187ecf
>>  but I suspect it's just a start.
>> How did you checked the project code for that?
>
> Per rG01b88dd66d9d52d3f531a7d490e18c549f36f445 
> , unused 
> `dyn_cast` results should turn into `isa`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D152495#4628028 , @goncharov wrote:

> there is a number of unused vaiables in conditional loops that are firing 
> now, I have submitted  
> https://github.com/llvm/llvm-project/commit/74f4daef0412be33002bd4e24a30cb47d0187ecf
>  but I suspect it's just a start.
> How did you checked the project code for that?

Per rG01b88dd66d9d52d3f531a7d490e18c549f36f445 
, unused 
`dyn_cast` results should turn into `isa`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Mikhail Goncharov via Phabricator via cfe-commits]

goncharov added a comment.

there is a number of unused vaiables in conditional loops there are firing now, 
I have submitted  
https://github.com/llvm/llvm-project/commit/74f4daef0412be33002bd4e24a30cb47d0187ecf
 but I suspect it's just a start.
How did you checked the project code for that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG92023b150990: Reland [Clang][SemaCXX] Add unused 
warning for variables declared in condition… (authored by hazohelet).

Changed prior to commit:
  https://reviews.llvm.org/D152495?vs=550443=554539#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/warn-unused-variables.cpp

Index: clang/test/SemaCXX/warn-unused-variables.cpp
===
--- clang/test/SemaCXX/warn-unused-variables.cpp
+++ clang/test/SemaCXX/warn-unused-variables.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++14-extensions -Wno-c++17-extensions -verify -std=c++11 %s
 template void f() {
   T t;
   t = 17;
@@ -294,3 +294,115 @@
 }
 
 } // namespace gh54489
+
+namespace inside_condition {
+  void ifs() {
+if (int hoge = 0) // expected-warning {{unused variable 'hoge'}}
+  return;
+if (const int const_hoge = 0) // expected-warning {{unused variable 'const_hoge'}}
+  return;
+else if (int fuga = 0)
+  (void)fuga;
+else if (int used = 1; int catched = used) // expected-warning {{unused variable 'catched'}}
+  return;
+else if (int refed = 1; int used = refed)
+  (void)used;
+else if (int unused1 = 2; int unused2 = 3) // expected-warning {{unused variable 'unused1'}} \
+   // expected-warning {{unused variable 'unused2'}}
+  return;
+else if (int unused = 4; int used = 5) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used = 6; int unused = 7) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used1 = 8; int used2 = 9)
+  (void)(used1 + used2);
+else if (auto [a, b] = (int[2]){ 1, 2 }; 1) // expected-warning {{unused variable '[a, b]'}}
+  return;
+else if (auto [a, b] = (int[2]){ 1, 2 }; a)
+  return;
+  }
+
+  void fors() {
+for (int i = 0;int unused = 0;); // expected-warning {{unused variable 'i'}} \
+ // expected-warning {{unused variable 'unused'}}
+for (int i = 0;int used = 0;) // expected-warning {{unused variable 'i'}}
+  (void)used;
+  while(int var = 1) // expected-warning {{unused variable 'var'}}
+return;
+  }
+
+  void whiles() {
+while(int unused = 1) // expected-warning {{unused variable 'unused'}}
+  return;
+while(int used = 1)
+  (void)used;
+  }
+
+
+  void switches() {
+switch(int unused = 1) { // expected-warning {{unused variable 'unused'}}
+  case 1: return;
+}
+switch(constexpr int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case used: return;
+}
+switch(int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case 3: (void)used;
+}
+switch(constexpr int used1 = 0; constexpr int used2 = 6) {
+  case (used1+used2): return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) { // expected-warning {{unused variable '[a, b]'}}
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; b) {
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) {
+  case 1: (void)a;
+}
+  }
+  template 
+  struct Vector {
+void doIt() {
+  for (auto& e : elements){} // expected-warning {{unused variable 'e'}}
+}
+T elements[10];
+  };
+  void ranged_for() {
+Vectorvector;
+vector.doIt(); // expected-note {{here}}
+  }
+
+
+  struct RAII {
+int 
+RAII(int ) : x(ref) {}
+~RAII() { x = 0;}
+operator int() const { return 1; }
+  };
+  void aggregate() {
+int x = 10;
+int y = 10;
+if (RAII var = x) {}
+for(RAII var = x; RAII var2 = y;) {}
+while (RAII var = x) {}
+switch (RAII var = x) {}
+  }
+
+  struct TrivialDtor{
+int 
+TrivialDtor(int ) : x(ref) { ref = 32; }
+operator int() const { return 1; }
+  };
+  void trivial_dtor() {
+int x = 10;
+int y = 10;
+if (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+for(TrivialDtor var = x; TrivialDtor var2 = y;) {} // expected-warning {{unused variable 'var'}} \
+ // expected-warning {{unused variable 'var2'}}
+while (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+switch (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+  }

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 550443.
hazohelet added a comment.

Removed warning fixes that are now in D158016 


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

https://reviews.llvm.org/D152495

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/warn-unused-variables.cpp

Index: clang/test/SemaCXX/warn-unused-variables.cpp
===
--- clang/test/SemaCXX/warn-unused-variables.cpp
+++ clang/test/SemaCXX/warn-unused-variables.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++14-extensions -Wno-c++17-extensions -verify -std=c++11 %s
 template void f() {
   T t;
   t = 17;
@@ -294,3 +294,115 @@
 }
 
 } // namespace gh54489
+
+namespace inside_condition {
+  void ifs() {
+if (int hoge = 0) // expected-warning {{unused variable 'hoge'}}
+  return;
+if (const int const_hoge = 0) // expected-warning {{unused variable 'const_hoge'}}
+  return;
+else if (int fuga = 0)
+  (void)fuga;
+else if (int used = 1; int catched = used) // expected-warning {{unused variable 'catched'}}
+  return;
+else if (int refed = 1; int used = refed)
+  (void)used;
+else if (int unused1 = 2; int unused2 = 3) // expected-warning {{unused variable 'unused1'}} \
+   // expected-warning {{unused variable 'unused2'}}
+  return;
+else if (int unused = 4; int used = 5) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used = 6; int unused = 7) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used1 = 8; int used2 = 9)
+  (void)(used1 + used2);
+else if (auto [a, b] = (int[2]){ 1, 2 }; 1) // expected-warning {{unused variable '[a, b]'}}
+  return;
+else if (auto [a, b] = (int[2]){ 1, 2 }; a)
+  return;
+  }
+
+  void fors() {
+for (int i = 0;int unused = 0;); // expected-warning {{unused variable 'i'}} \
+ // expected-warning {{unused variable 'unused'}}
+for (int i = 0;int used = 0;) // expected-warning {{unused variable 'i'}}
+  (void)used;
+  while(int var = 1) // expected-warning {{unused variable 'var'}}
+return;
+  }
+
+  void whiles() {
+while(int unused = 1) // expected-warning {{unused variable 'unused'}}
+  return;
+while(int used = 1)
+  (void)used;
+  }
+
+
+  void switches() {
+switch(int unused = 1) { // expected-warning {{unused variable 'unused'}}
+  case 1: return;
+}
+switch(constexpr int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case used: return;
+}
+switch(int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case 3: (void)used;
+}
+switch(constexpr int used1 = 0; constexpr int used2 = 6) {
+  case (used1+used2): return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) { // expected-warning {{unused variable '[a, b]'}}
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; b) {
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) {
+  case 1: (void)a;
+}
+  }
+  template 
+  struct Vector {
+void doIt() {
+  for (auto& e : elements){} // expected-warning {{unused variable 'e'}}
+}
+T elements[10];
+  };
+  void ranged_for() {
+Vectorvector;
+vector.doIt(); // expected-note {{here}}
+  }
+
+
+  struct RAII {
+int 
+RAII(int ) : x(ref) {}
+~RAII() { x = 0;}
+operator int() const { return 1; }
+  };
+  void aggregate() {
+int x = 10;
+int y = 10;
+if (RAII var = x) {}
+for(RAII var = x; RAII var2 = y;) {}
+while (RAII var = x) {}
+switch (RAII var = x) {}
+  }
+
+  struct TrivialDtor{
+int 
+TrivialDtor(int ) : x(ref) { ref = 32; }
+operator int() const { return 1; }
+  };
+  void trivial_dtor() {
+int x = 10;
+int y = 10;
+if (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+for(TrivialDtor var = x; TrivialDtor var2 = y;) {} // expected-warning {{unused variable 'var'}} \
+ // expected-warning {{unused variable 'var2'}}
+while (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+switch (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+  }
+
+} // namespace inside_condition
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- 

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

Please split the warning fixes off into a separate patch.


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 550433.
hazohelet added a comment.
Herald added subscribers: llvm-commits, wangpc, hoy, wlei, steakhal, abrachet, 
ormris, martong, MaskRay, hiraditya.
Herald added a reviewer: NoQ.
Herald added projects: LLVM, lld-macho.
Herald added a reviewer: lld-macho.

Fixed newly-issued warnings when trying 
`check-clang,llvm,lld,compiler-rt,cxx,cxxabi` with `-Werror=unused-variable` 
locally using local build of clang


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

https://reviews.llvm.org/D152495

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/AST/Interp/ByteCodeStmtGen.cpp
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CGExprConstant.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Yaml.h
  clang/test/SemaCXX/warn-unused-variables.cpp
  clang/tools/clang-scan-deps/ClangScanDeps.cpp
  lld/MachO/Driver.cpp
  llvm/lib/Bitcode/Reader/MetadataLoader.cpp
  llvm/lib/DebugInfo/DWARF/DWARFContext.cpp
  llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
  llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
  llvm/lib/IR/PrintPasses.cpp
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/lib/Support/VirtualFileSystem.cpp
  llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
  llvm/tools/llvm-profgen/ProfiledBinary.cpp

Index: llvm/tools/llvm-profgen/ProfiledBinary.cpp
===
--- llvm/tools/llvm-profgen/ProfiledBinary.cpp
+++ llvm/tools/llvm-profgen/ProfiledBinary.cpp
@@ -812,7 +812,7 @@
   // Handles DWO sections that can either be in .o, .dwo or .dwp files.
   for (const auto  : DebugContext->compile_units()) {
 DWARFUnit *const DwarfUnit = CompilationUnit.get();
-if (std::optional DWOId = DwarfUnit->getDWOId()) {
+if (DwarfUnit->getDWOId()) {
   DWARFUnit *DWOCU = DwarfUnit->getNonSkeletonUnitDIE(false).getDwarfUnit();
   if (!DWOCU->isDWOUnit()) {
 std::string DWOName = dwarf::toString(
Index: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
===
--- llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -9178,7 +9178,7 @@
 VecOp = FMulRecipe;
   } else {
 if (RecurrenceDescriptor::isMinMaxRecurrenceKind(Kind)) {
-  if (auto *Cmp = dyn_cast(CurrentLink)) {
+  if (isa(CurrentLink)) {
 assert(isa(CurrentLinkI) &&
"need to have the compare of the select");
 continue;
Index: llvm/lib/Support/VirtualFileSystem.cpp
===
--- llvm/lib/Support/VirtualFileSystem.cpp
+++ llvm/lib/Support/VirtualFileSystem.cpp
@@ -1337,7 +1337,7 @@
   SmallString<256> Path;
   Path_.toVector(Path);
 
-  if (std::error_code EC = makeCanonical(Path))
+  if (makeCanonical(Path))
 return {};
 
   return ExternalFS->isLocal(Path, Result);
Index: llvm/lib/Passes/StandardInstrumentations.cpp
===
--- llvm/lib/Passes/StandardInstrumentations.cpp
+++ llvm/lib/Passes/StandardInstrumentations.cpp
@@ -501,7 +501,7 @@
   static SmallVector FD{-1};
   SmallVector SR{S};
   static SmallVector FileName{""};
-  if (auto Err = prepareTempFiles(FD, SR, FileName)) {
+  if (prepareTempFiles(FD, SR, FileName)) {
 dbgs() << "Unable to create temporary file.";
 return;
   }
@@ -518,7 +518,7 @@
 return;
   }
 
-  if (auto Err = cleanUpTempFiles(FileName))
+  if (cleanUpTempFiles(FileName))
 dbgs() << "Unable to remove temporary file.";
 }
 
Index: llvm/lib/IR/PrintPasses.cpp
===
--- llvm/lib/IR/PrintPasses.cpp
+++ llvm/lib/IR/PrintPasses.cpp
@@ -212,7 +212,7 @@
   static SmallVector FD{-1, -1, -1};
   SmallVector SR{Before, After};
   static SmallVector FileName{"", "", ""};
-  if (auto Err = prepareTempFiles(FD, SR, FileName))
+  if (prepareTempFiles(FD, SR, FileName))
 return "Unable to create temporary file.";
 
   static ErrorOr DiffExe = sys::findProgramByName(DiffBinary);
@@ -238,7 +238,7 @@
   else
 return "Unable to read result.";
 
-  if (auto Err = cleanUpTempFiles(FileName))
+  if (cleanUpTempFiles(FileName))
 return "Unable to remove temporary file.";
 
   return Diff;
Index: llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
===
--- llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
+++ llvm/lib/DebugInfo/GSYM/DwarfTransformer.cpp
@@ -462,7 +462,7 @@
   size_t NumBefore = Gsym.getNumFunctionInfos();
   auto getDie = [&](DWARFUnit ) -> DWARFDie {

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D152495#4587804 , @chapuni wrote:

> Would this cause many warnings in Clang/LLVM tree?
> https://lab.llvm.org/buildbot/#/builders/36/builds/36560
>
> I hope you to fix possible warnings at first.

Thanks. I'll revert this change.
I was thinking of pushing speculative fixes upstream, but the lines that need 
fixing are not only a few lines, so I now think it needs a review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[NAKAMURA Takumi via Phabricator via cfe-commits]

chapuni added a comment.

Would this cause many warnings in Clang/LLVM tree?
https://lab.llvm.org/buildbot/#/builders/36/builds/36560

I hope you to fix possible warnings at first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D152495#4566634 , @aaron.ballman 
wrote:

> In D152495#4563520 , @hazohelet 
> wrote:
>
>> I reverted this change because it broke sanitizer buildbots.
>> https://lab.llvm.org/buildbot/#/builders/19/builds/18369
>> The cause of the errors here are relatively clear and I can speculatively 
>> replace `if (std::error_code EC = makeCanonical(Path))` with `if 
>> (makeCanonical(Path))`
>
> That changes makes sense to me.
>
>> https://lab.llvm.org/buildbot/#/builders/94/builds/15865
>> https://lab.llvm.org/buildbot/#/builders/240/builds/12947
>> These 2 are not clear for me, so I'll take time to find the cause.
>
> Neither of these are problems with your patch -- they're flaky tests in this 
> case.

Thanks! I'll reland this patch with the unused `std::error_code` fix. I'll 
revert again if the same CI keeps failing for a few hours.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D152495#4563520 , @hazohelet wrote:

> I reverted this change because it broke sanitizer buildbots.
> https://lab.llvm.org/buildbot/#/builders/19/builds/18369
> The cause of the errors here are relatively clear and I can speculatively 
> replace `if (std::error_code EC = makeCanonical(Path))` with `if 
> (makeCanonical(Path))`

That changes makes sense to me.

> https://lab.llvm.org/buildbot/#/builders/94/builds/15865
> https://lab.llvm.org/buildbot/#/builders/240/builds/12947
> These 2 are not clear for me, so I'll take time to find the cause.

Neither of these are problems with your patch -- they're flaky tests in this 
case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

The fuzzer timeouts fail all the time; I usually just assume it's unrelated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

I reverted this change because it broke sanitizer buildbots.
https://lab.llvm.org/buildbot/#/builders/19/builds/18369
The cause of the errors here are relatively clear and I can speculatively 
replace `if (std::error_code EC = makeCanonical(Path))` with `if 
(makeCanonical(Path))`

https://lab.llvm.org/buildbot/#/builders/94/builds/15865
https://lab.llvm.org/buildbot/#/builders/240/builds/12947
These 2 are not clear for me, so I'll take time to find the cause.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbd0ed0abc31f: [Clang][SemaCXX] Add unused warning for 
variables declared in condition… (authored by hazohelet).

Changed prior to commit:
  https://reviews.llvm.org/D152495?vs=540479=547525#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/warn-unused-variables.cpp

Index: clang/test/SemaCXX/warn-unused-variables.cpp
===
--- clang/test/SemaCXX/warn-unused-variables.cpp
+++ clang/test/SemaCXX/warn-unused-variables.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++14-extensions -Wno-c++17-extensions -verify -std=c++11 %s
 template void f() {
   T t;
   t = 17;
@@ -294,3 +294,115 @@
 }
 
 } // namespace gh54489
+
+namespace inside_condition {
+  void ifs() {
+if (int hoge = 0) // expected-warning {{unused variable 'hoge'}}
+  return;
+if (const int const_hoge = 0) // expected-warning {{unused variable 'const_hoge'}}
+  return;
+else if (int fuga = 0)
+  (void)fuga;
+else if (int used = 1; int catched = used) // expected-warning {{unused variable 'catched'}}
+  return;
+else if (int refed = 1; int used = refed)
+  (void)used;
+else if (int unused1 = 2; int unused2 = 3) // expected-warning {{unused variable 'unused1'}} \
+   // expected-warning {{unused variable 'unused2'}}
+  return;
+else if (int unused = 4; int used = 5) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used = 6; int unused = 7) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used1 = 8; int used2 = 9)
+  (void)(used1 + used2);
+else if (auto [a, b] = (int[2]){ 1, 2 }; 1) // expected-warning {{unused variable '[a, b]'}}
+  return;
+else if (auto [a, b] = (int[2]){ 1, 2 }; a)
+  return;
+  }
+
+  void fors() {
+for (int i = 0;int unused = 0;); // expected-warning {{unused variable 'i'}} \
+ // expected-warning {{unused variable 'unused'}}
+for (int i = 0;int used = 0;) // expected-warning {{unused variable 'i'}}
+  (void)used;
+  while(int var = 1) // expected-warning {{unused variable 'var'}}
+return;
+  }
+
+  void whiles() {
+while(int unused = 1) // expected-warning {{unused variable 'unused'}}
+  return;
+while(int used = 1)
+  (void)used;
+  }
+
+
+  void switches() {
+switch(int unused = 1) { // expected-warning {{unused variable 'unused'}}
+  case 1: return;
+}
+switch(constexpr int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case used: return;
+}
+switch(int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case 3: (void)used;
+}
+switch(constexpr int used1 = 0; constexpr int used2 = 6) {
+  case (used1+used2): return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) { // expected-warning {{unused variable '[a, b]'}}
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; b) {
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) {
+  case 1: (void)a;
+}
+  }
+  template 
+  struct Vector {
+void doIt() {
+  for (auto& e : elements){} // expected-warning {{unused variable 'e'}}
+}
+T elements[10];
+  };
+  void ranged_for() {
+Vectorvector;
+vector.doIt(); // expected-note {{here}}
+  }
+
+
+  struct RAII {
+int 
+RAII(int ) : x(ref) {}
+~RAII() { x = 0;}
+operator int() const { return 1; }
+  };
+  void aggregate() {
+int x = 10;
+int y = 10;
+if (RAII var = x) {}
+for(RAII var = x; RAII var2 = y;) {}
+while (RAII var = x) {}
+switch (RAII var = x) {}
+  }
+
+  struct TrivialDtor{
+int 
+TrivialDtor(int ) : x(ref) { ref = 32; }
+operator int() const { return 1; }
+  };
+  void trivial_dtor() {
+int x = 10;
+int y = 10;
+if (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+for(TrivialDtor var = x; TrivialDtor var2 = y;) {} // expected-warning {{unused variable 'var'}} \
+ // expected-warning {{unused variable 'var2'}}
+while (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+switch (TrivialDtor var = x) {} 

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin accepted this revision.
cor3ntin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks. Make sure you rebase the changes to the release notes


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

@cor3ntin It's next week :)


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4016-4019
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);

cor3ntin wrote:
> hazohelet wrote:
> > cor3ntin wrote:
> > > hazohelet wrote:
> > > > cor3ntin wrote:
> > > > > aaron.ballman wrote:
> > > > > > `isAggregateType()` includes arrays and I think we still want to 
> > > > > > diagnose an unused array.
> > > > > Should it not be `Condition->hasSideEffects()` ? 
> > > > > Hopefully we want to be consistent with existing behavior 
> > > > > https://godbolt.org/z/6abbPhn4G
> > > > The condition here only applies to condition variables.
> > > > C++ does not allow array type in condition variable, correct? I think 
> > > > it's OK to use `isAggregateType` as well then.
> > > After a chat with Aaron, I have a few questions: Do we call 
> > > `Scope::AddDecl` - or `PushOnScopeChains` somewhere?
> > > My intuition is that if the condition is added to a scope, and we call `  
> > >   ConditionVar->setReferenced(false);`, the all the diagnostic mechanism 
> > > in `DiagnoseUnusedDecl` should be triggered. and it seen to be because we 
> > > do get a diagnostic.
> > > So neither `if (!T->isAggregateType())` or any check should be needed 
> > > there
> > > 
> > > Can you just remove that if statement (just set `setReferenced(false)`) 
> > > and see if it works? Otherwise, i think we need to understand why it 
> > > doesn't but trying to reimplement the logic of 
> > > `ShouldDiagnoseUnusedDecl`seems fraught with unbounded complexity.
> > It didn't break any tests locally, but when the record type does not have a 
> > user-provided dtor, it is diagnosed as unused, which seems not nice. 
> > (example written in the test `trivial_dtor`)
> It's still a declaration that does nothing, so diagnosing let you remove it
> https://compiler-explorer.com/z/a1GGs9cxa this is consistent with local 
> variables. I like the simplicity of the change.
Ah, I see. Thanks!


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

This looks reasonable to me now.
I'll wait next week before approving it so other get a chance to look at it :)




Comment at: clang/lib/Sema/SemaExprCXX.cpp:4016-4019
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);

hazohelet wrote:
> cor3ntin wrote:
> > hazohelet wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > `isAggregateType()` includes arrays and I think we still want to 
> > > > > diagnose an unused array.
> > > > Should it not be `Condition->hasSideEffects()` ? 
> > > > Hopefully we want to be consistent with existing behavior 
> > > > https://godbolt.org/z/6abbPhn4G
> > > The condition here only applies to condition variables.
> > > C++ does not allow array type in condition variable, correct? I think 
> > > it's OK to use `isAggregateType` as well then.
> > After a chat with Aaron, I have a few questions: Do we call 
> > `Scope::AddDecl` - or `PushOnScopeChains` somewhere?
> > My intuition is that if the condition is added to a scope, and we call `
> > ConditionVar->setReferenced(false);`, the all the diagnostic mechanism in 
> > `DiagnoseUnusedDecl` should be triggered. and it seen to be because we do 
> > get a diagnostic.
> > So neither `if (!T->isAggregateType())` or any check should be needed there
> > 
> > Can you just remove that if statement (just set `setReferenced(false)`) and 
> > see if it works? Otherwise, i think we need to understand why it doesn't 
> > but trying to reimplement the logic of `ShouldDiagnoseUnusedDecl`seems 
> > fraught with unbounded complexity.
> It didn't break any tests locally, but when the record type does not have a 
> user-provided dtor, it is diagnosed as unused, which seems not nice. (example 
> written in the test `trivial_dtor`)
It's still a declaration that does nothing, so diagnosing let you remove it
https://compiler-explorer.com/z/a1GGs9cxa this is consistent with local 
variables. I like the simplicity of the change.


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4016-4019
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);

cor3ntin wrote:
> hazohelet wrote:
> > cor3ntin wrote:
> > > aaron.ballman wrote:
> > > > `isAggregateType()` includes arrays and I think we still want to 
> > > > diagnose an unused array.
> > > Should it not be `Condition->hasSideEffects()` ? 
> > > Hopefully we want to be consistent with existing behavior 
> > > https://godbolt.org/z/6abbPhn4G
> > The condition here only applies to condition variables.
> > C++ does not allow array type in condition variable, correct? I think it's 
> > OK to use `isAggregateType` as well then.
> After a chat with Aaron, I have a few questions: Do we call `Scope::AddDecl` 
> - or `PushOnScopeChains` somewhere?
> My intuition is that if the condition is added to a scope, and we call `
> ConditionVar->setReferenced(false);`, the all the diagnostic mechanism in 
> `DiagnoseUnusedDecl` should be triggered. and it seen to be because we do get 
> a diagnostic.
> So neither `if (!T->isAggregateType())` or any check should be needed there
> 
> Can you just remove that if statement (just set `setReferenced(false)`) and 
> see if it works? Otherwise, i think we need to understand why it doesn't but 
> trying to reimplement the logic of `ShouldDiagnoseUnusedDecl`seems fraught 
> with unbounded complexity.
It didn't break any tests locally, but when the record type does not have a 
user-provided dtor, it is diagnosed as unused, which seems not nice. (example 
written in the test `trivial_dtor`)


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 540479.
hazohelet marked 6 inline comments as done.
hazohelet added a comment.

Address comments from Aaron and Corentin

- Call `ConditionVar->setReferenced(false)` without any checks
- Added some more tests


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

https://reviews.llvm.org/D152495

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/warn-unused-variables.cpp

Index: clang/test/SemaCXX/warn-unused-variables.cpp
===
--- clang/test/SemaCXX/warn-unused-variables.cpp
+++ clang/test/SemaCXX/warn-unused-variables.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++14-extensions -Wno-c++17-extensions -verify -std=c++11 %s
 template void f() {
   T t;
   t = 17;
@@ -294,3 +294,115 @@
 }
 
 } // namespace gh54489
+
+namespace inside_condition {
+  void ifs() {
+if (int hoge = 0) // expected-warning {{unused variable 'hoge'}}
+  return;
+if (const int const_hoge = 0) // expected-warning {{unused variable 'const_hoge'}}
+  return;
+else if (int fuga = 0)
+  (void)fuga;
+else if (int used = 1; int catched = used) // expected-warning {{unused variable 'catched'}}
+  return;
+else if (int refed = 1; int used = refed)
+  (void)used;
+else if (int unused1 = 2; int unused2 = 3) // expected-warning {{unused variable 'unused1'}} \
+   // expected-warning {{unused variable 'unused2'}}
+  return;
+else if (int unused = 4; int used = 5) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used = 6; int unused = 7) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used1 = 8; int used2 = 9)
+  (void)(used1 + used2);
+else if (auto [a, b] = (int[2]){ 1, 2 }; 1) // expected-warning {{unused variable '[a, b]'}}
+  return;
+else if (auto [a, b] = (int[2]){ 1, 2 }; a)
+  return;
+  }
+
+  void fors() {
+for (int i = 0;int unused = 0;); // expected-warning {{unused variable 'i'}} \
+ // expected-warning {{unused variable 'unused'}}
+for (int i = 0;int used = 0;) // expected-warning {{unused variable 'i'}}
+  (void)used;
+  while(int var = 1) // expected-warning {{unused variable 'var'}}
+return;
+  }
+
+  void whiles() {
+while(int unused = 1) // expected-warning {{unused variable 'unused'}}
+  return;
+while(int used = 1)
+  (void)used;
+  }
+
+
+  void switches() {
+switch(int unused = 1) { // expected-warning {{unused variable 'unused'}}
+  case 1: return;
+}
+switch(constexpr int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case used: return;
+}
+switch(int used = 3; int unused = 4) { // expected-warning {{unused variable 'unused'}}
+  case 3: (void)used;
+}
+switch(constexpr int used1 = 0; constexpr int used2 = 6) {
+  case (used1+used2): return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) { // expected-warning {{unused variable '[a, b]'}}
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; b) {
+  case 1: return;
+}
+switch(auto [a, b] = (int[2]){ 1, 2 }; 1) {
+  case 1: (void)a;
+}
+  }
+  template 
+  struct Vector {
+void doIt() {
+  for (auto& e : elements){} // expected-warning {{unused variable 'e'}}
+}
+T elements[10];
+  };
+  void ranged_for() {
+Vectorvector;
+vector.doIt(); // expected-note {{here}}
+  }
+
+
+  struct RAII {
+int 
+RAII(int ) : x(ref) {}
+~RAII() { x = 0;}
+operator int() const { return 1; }
+  };
+  void aggregate() {
+int x = 10;
+int y = 10;
+if (RAII var = x) {}
+for(RAII var = x; RAII var2 = y;) {}
+while (RAII var = x) {}
+switch (RAII var = x) {}
+  }
+
+  struct TrivialDtor{
+int 
+TrivialDtor(int ) : x(ref) { ref = 32; }
+operator int() const { return 1; }
+  };
+  void trivial_dtor() {
+int x = 10;
+int y = 10;
+if (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+for(TrivialDtor var = x; TrivialDtor var2 = y;) {} // expected-warning {{unused variable 'var'}} \
+ // expected-warning {{unused variable 'var2'}}
+while (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+switch (TrivialDtor var = x) {} // expected-warning {{unused variable 'var'}}
+  }
+
+} // namespace inside_condition
Index: 

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4016-4019
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);

hazohelet wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > `isAggregateType()` includes arrays and I think we still want to diagnose 
> > > an unused array.
> > Should it not be `Condition->hasSideEffects()` ? 
> > Hopefully we want to be consistent with existing behavior 
> > https://godbolt.org/z/6abbPhn4G
> The condition here only applies to condition variables.
> C++ does not allow array type in condition variable, correct? I think it's OK 
> to use `isAggregateType` as well then.
After a chat with Aaron, I have a few questions: Do we call `Scope::AddDecl` - 
or `PushOnScopeChains` somewhere?
My intuition is that if the condition is added to a scope, and we call `
ConditionVar->setReferenced(false);`, the all the diagnostic mechanism in 
`DiagnoseUnusedDecl` should be triggered. and it seen to be because we do get a 
diagnostic.
So neither `if (!T->isAggregateType())` or any check should be needed there

Can you just remove that if statement (just set `setReferenced(false)`) and see 
if it works? Otherwise, i think we need to understand why it doesn't but trying 
to reimplement the logic of `ShouldDiagnoseUnusedDecl`seems fraught with 
unbounded complexity.


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet marked an inline comment as done.
hazohelet added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4016-4019
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);

cor3ntin wrote:
> aaron.ballman wrote:
> > `isAggregateType()` includes arrays and I think we still want to diagnose 
> > an unused array.
> Should it not be `Condition->hasSideEffects()` ? 
> Hopefully we want to be consistent with existing behavior 
> https://godbolt.org/z/6abbPhn4G
The condition here only applies to condition variables.
C++ does not allow array type in condition variable, correct? I think it's OK 
to use `isAggregateType` as well then.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5348
   NewVar->getDeclContext()->isFunctionOrMethod() &&
-  OldVar->getType()->isDependentType())
+  OldVar->getType()->isDependentType() && !OldVar->isImplicit())
 DiagnoseUnusedDecl(NewVar);

aaron.ballman wrote:
> Which test case covers this change?
I wanted to suppress warnings against implicitly declared variables in range 
statements like `__range2`. I'll add a test for this.


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4016-4019
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);

aaron.ballman wrote:
> `isAggregateType()` includes arrays and I think we still want to diagnose an 
> unused array.
Should it not be `Condition->hasSideEffects()` ? 
Hopefully we want to be consistent with existing behavior 
https://godbolt.org/z/6abbPhn4G


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4016-4019
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);

`isAggregateType()` includes arrays and I think we still want to diagnose an 
unused array.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5348
   NewVar->getDeclContext()->isFunctionOrMethod() &&
-  OldVar->getType()->isDependentType())
+  OldVar->getType()->isDependentType() && !OldVar->isImplicit())
 DiagnoseUnusedDecl(NewVar);

Which test case covers this change?



Comment at: clang/test/SemaCXX/warn-unused-variables.cpp:2
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label 
-Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label 
-Wno-c++1y-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label 
-Wno-c++1y-extensions -Wno-c++1z-extensions -verify -std=c++11 %s
 template void f() {





Comment at: clang/test/SemaCXX/warn-unused-variables.cpp:317-318
+  (void)used;
+else if (int used1 = 8; int used2 = 9)
+  (void)(used1 + used2);
+  }

This should diagnose the decomposition declaration as being unused, but that 
already works today (so let's make sure we don't get duplicate diagnostics for 
this case): https://godbolt.org/z/6jP6sW1x6

An additional test case after this would be to add:
```
else if (auto [a, b] = (int[2]){ 1, 2 }; a)
  return;
```
which should get no diagnostics.

We should also have these tests for the other kinds of conditions (for, switch).



Comment at: clang/test/SemaCXX/warn-unused-variables.cpp:321
+
+  void fors() {
+for (int i = 0;int unused = 0;); // expected-warning {{unused variable 
'i'}} \

Can you also add coverage for `switch`?


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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet updated this revision to Diff 538591.
hazohelet marked 5 inline comments as done.
hazohelet added a comment.

- Stop marking condition variable declarations as unused and diagnose all 
`VarDecl` that is marked used but not referenced
- Added test not to warn when the declaration is not aggregate type
- NFC changes (comments, typo)


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

https://reviews.llvm.org/D152495

Files:
  clang/docs/ReleaseNotes.rst
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/SemaCXX/warn-unused-variables.cpp

Index: clang/test/SemaCXX/warn-unused-variables.cpp
===
--- clang/test/SemaCXX/warn-unused-variables.cpp
+++ clang/test/SemaCXX/warn-unused-variables.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -Wno-c++1z-extensions -verify -std=c++11 %s
 template void f() {
   T t;
   t = 17;
@@ -294,3 +294,49 @@
 }
 
 } // namespace gh54489
+
+namespace inside_condition {
+  void ifs() {
+if (int hoge = 0) // expected-warning {{unused variable 'hoge'}}
+  return;
+if (const int const_hoge = 0) // expected-warning {{unused variable 'const_hoge'}}
+  return;
+else if (int fuga = 0)
+  (void)fuga;
+else if (int used = 1; int catched = used) // expected-warning {{unused variable 'catched'}}
+  return;
+else if (int refed = 1; int used = refed)
+  (void)used;
+else if (int unused1 = 2; int unused2 = 3) // expected-warning {{unused variable 'unused1'}} \
+   // expected-warning {{unused variable 'unused2'}}
+  return;
+else if (int unused = 4; int used = 5) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used = 6; int unused = 7) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used1 = 8; int used2 = 9)
+  (void)(used1 + used2);
+  }
+
+  void fors() {
+for (int i = 0;int unused = 0;); // expected-warning {{unused variable 'i'}} \
+ // expected-warning {{unused variable 'unused'}}
+for (int i = 0;int used = 0;) // expected-warning {{unused variable 'i'}}
+  (void)used;
+  while(int var = 1) // expected-warning {{unused variable 'var'}}
+return;
+  }
+
+  struct RAII {
+int 
+RAII(int ) : x(ref) {}
+~RAII() { x = 0;}
+operator bool() const { return true; }
+  };
+  void aggregate() {
+int x = 10;
+if (RAII var = x) {}
+  }
+
+
+} // namespace inside_condition
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5345,7 +5345,7 @@
   // will have been deferred.
   if (!NewVar->isInvalidDecl() &&
   NewVar->getDeclContext()->isFunctionOrMethod() &&
-  OldVar->getType()->isDependentType())
+  OldVar->getType()->isDependentType() && !OldVar->isImplicit())
 DiagnoseUnusedDecl(NewVar);
 }
 
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -4013,6 +4013,11 @@
   ConditionVar, ConditionVar->getType().getNonReferenceType(), VK_LValue,
   ConditionVar->getLocation());
 
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced later. e.g.: if (int var = init());
+  if (!T->isAggregateType())
+ConditionVar->setReferenced(/*R=*/false);
+
   switch (CK) {
   case ConditionKind::Boolean:
 return CheckBooleanCondition(StmtLoc, Condition.get());
Index: clang/lib/Sema/SemaDecl.cpp
===
--- clang/lib/Sema/SemaDecl.cpp
+++ clang/lib/Sema/SemaDecl.cpp
@@ -1989,7 +1989,7 @@
 return false;
   } else if (!D->getDeclName()) {
 return false;
-  } else if (D->isReferenced() || D->isUsed()) {
+  } else if (D->isReferenced() || (!isa(D) && D->isUsed())) {
 return false;
   }
 
Index: clang/docs/ReleaseNotes.rst
===
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -381,6 +381,9 @@
   (`#57081: `_)
 - Clang no longer emits inappropriate notes about the loss of ``__unaligned`` qualifier
   on overload resolution, when the actual reason for the failure is loss of other qualifiers.
+- Clang now warns on unused variables of non-aggregate types declared and 

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

In D152495#4481588 , @aaron.ballman 
wrote:

> This is a bit of an odd situation -- the condition variable *is* used (e.g., 
> its value is loaded to determine whether to go into the if branch or the else 
> branch), so we should not be marking it as unused in 
> `Sema::CheckConditionVariable()`. Instead, I think we need to decide what 
> constitutes "unused" in this case. Consider:
>
>   struct RAII {
> int 
>   
> RAII(int ) : x(ref) {}
> ~RAII() { x = 0; }
>   
> operator bool() const { return true; }
>   };
>   
>   void func() {
> int i = 12;
> if (RAII name{i}) {
> }
>   }
>
> I don't think `name` is unused in the same way that `i` is unused in `if (int 
> i = 1) {}`. So I think we only want this to apply to scalars and not class 
> types. I think the changes should be happening in 
> `Sema::ShouldDiagnoseUnusedDecl()`  (and you might need to thread the `Scope` 
> object through to that call -- my naive thinking is that a variable 
> declaration in a condition scope should be diagnosed as unused if it is used 
> but not referenced, but I could be off-base).

Thank you for the insightful feedback!
I agree that we should not be emitting Wunused warnings in condition 
expressions when the declaration is of class type.

About the `Scope` object, if you mean `ConditionVarScope` flag by "condition 
scope", it seems to be used only for the condition of for-statements. 
(https://github.com/llvm/llvm-project/blob/f36b0169b8821e431644cb240f081538eb2b55ef/clang/lib/Parse/ParseExprCXX.cpp#L2045-L2057)
 Also it does not push new scope but modifies the flag of the current scope.
So it does not seem to be usable for our purpose here.

Instead, I think used-but-not-referenced variable declarations can be diagnosed 
as unused regardless of whether they are declared in condition expressions. 
This saves us from deleting the assertion from CodeGen and modifying AST tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is a bit of an odd situation -- the condition variable *is* used (e.g., 
its value is loaded to determine whether to go into the if branch or the else 
branch), so we should not be marking it as unused in 
`Sema::CheckConditionVariable()`. Instead, I think we need to decide what 
constitutes "unused" in this case. Consider:

  struct RAII {
int 
  
RAII(int ) : x(ref) {}
~RAII() { x = 0; }
  
operator bool() const { return true; }
  };
  
  void func() {
int i = 12;
if (RAII name{i}) {
}
  }

I don't think `name` is unused in the same way that `i` is unused in `if (int i 
= 1) {}`. So I think we only want this to apply to scalars and not class types. 
I think the changes should be happening in `Sema::ShouldDiagnoseUnusedDecl()`  
(and you might need to thread the `Scope` object through to that call -- my 
naive thinking is that a variable declaration in a condition scope should be 
diagnosed as unused if it is used but not referenced, but I could be off-base).




Comment at: clang/docs/ReleaseNotes.rst:340
   with ``__attribute__((cleanup(...)))`` to match GCC's behavior.
+- Clang now warns on unused variables declaread and initialized in condition
+  expressions.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4003-4004
+  // that are not referenced or used later. e.g.: if (int var = init());
+  ConditionVar->setReferenced(false);
+  ConditionVar->setIsUsed(false);
+

tbaeder wrote:
> shafik wrote:
> > 
> That seem a bit over the top given that it's the only boolean parameter.
Yeah, in this case it is blindingly obvious but I do see many cases in the code 
where folks use `bugprone-argument-comment` for single bool parameters and I 
just use it everywhere for consistency sake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4003-4004
+  // that are not referenced or used later. e.g.: if (int var = init());
+  ConditionVar->setReferenced(false);
+  ConditionVar->setIsUsed(false);
+

shafik wrote:
> 
That seem a bit over the top given that it's the only boolean parameter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik added inline comments.



Comment at: clang/lib/Sema/SemaExprCXX.cpp:4003-4004
+  // that are not referenced or used later. e.g.: if (int var = init());
+  ConditionVar->setReferenced(false);
+  ConditionVar->setIsUsed(false);
+




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added inline comments.



Comment at: clang/lib/CodeGen/CGExpr.cpp:2852
- "Should not use decl without marking it used!");
-
   if (ND->hasAttr()) {

Question for other reviewers: If we want to keep this assertion, would it 
instead make sense to mark the decl as used after emitting the diagnostic, in 
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L2154
 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152495

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

[PATCH] D152495: [Clang][SemaCXX] Add unused warning for variables declared in condition expressions

[Takuya Shimizu via Phabricator via cfe-commits]

hazohelet created this revision.
hazohelet added reviewers: aaron.ballman, tbaeder, shafik.
Herald added a project: All.
hazohelet requested review of this revision.
Herald added a project: clang.

This patch marks the declarations with initializations in condition expressions 
such as
`if (int var = init)` as unused and unreferenced so that `-Wunused` can warn on 
them.

Fixes https://github.com/llvm/llvm-project/issues/61681


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152495

Files:
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/DeclBase.h
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaExprCXX.cpp
  clang/test/AST/ast-dump-if-json.cpp
  clang/test/AST/ast-dump-stmt-json.cpp
  clang/test/AST/ast-dump-stmt.cpp
  clang/test/SemaCXX/warn-unused-variables.cpp

Index: clang/test/SemaCXX/warn-unused-variables.cpp
===
--- clang/test/SemaCXX/warn-unused-variables.cpp
+++ clang/test/SemaCXX/warn-unused-variables.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify %s
-// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -verify -std=c++11 %s
+// RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -Wno-c++1y-extensions -Wno-c++1z-extensions -verify -std=c++11 %s
 template void f() {
   T t;
   t = 17;
@@ -294,3 +294,36 @@
 }
 
 } // namespace gh54489
+
+namespace inside_condition {
+  void ifs() {
+if (int hoge = 0) // expected-warning {{unused variable 'hoge'}}
+  return;
+if (const int const_hoge = 0) // expected-warning {{unused variable 'const_hoge'}}
+  return;
+else if (int fuga = 0)
+  (void)fuga;
+else if (int used = 1; int catched = used) // expected-warning {{unused variable 'catched'}}
+  return;
+else if (int refed = 1; int used = refed)
+  (void)used;
+else if (int unused1 = 2; int unused2 = 3) // expected-warning {{unused variable 'unused1'}} \
+   // expected-warning {{unused variable 'unused2'}}
+  return;
+else if (int unused = 4; int used = 5) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used = 6; int unused = 7) // expected-warning {{unused variable 'unused'}}
+  (void)used;
+else if (int used1 = 8; int used2 = 9)
+  (void)(used1 + used2);
+  }
+
+  void fors() {
+for (int i = 0;int unused = 0;); // expected-warning {{unused variable 'i'}} \
+ // expected-warning {{unused variable 'unused'}}
+for (int i = 0;int used = 0;) // expected-warning {{unused variable 'i'}}
+  (void)used;
+  while(int var = 1) // expected-warning {{unused variable 'var'}}
+return;
+  }
+} // namespace inside_condition
Index: clang/test/AST/ast-dump-stmt.cpp
===
--- clang/test/AST/ast-dump-stmt.cpp
+++ clang/test/AST/ast-dump-stmt.cpp
@@ -179,7 +179,7 @@
   // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:12 used i 'int' cinit
   // CHECK-NEXT: IntegerLiteral 0x{{[^ ]*}}  'int' 0
   // CHECK-NEXT: DeclStmt
-  // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:23 used j 'int' cinit
+  // CHECK-NEXT: VarDecl 0x{{[^ ]*}}  col:23 j 'int' cinit
   // CHECK-NEXT: ImplicitCastExpr
   // CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}}  'int' lvalue Var 0x{{[^ ]*}} 'i' 'int'
   // CHECK-NEXT: ImplicitCastExpr 0x{{[^ ]*}}  'bool' 
Index: clang/test/AST/ast-dump-stmt-json.cpp
===
--- clang/test/AST/ast-dump-stmt-json.cpp
+++ clang/test/AST/ast-dump-stmt-json.cpp
@@ -4167,7 +4167,6 @@
 // CHECK-NEXT:"tokLen": 1
 // CHECK-NEXT:   }
 // CHECK-NEXT:  },
-// CHECK-NEXT:  "isUsed": true,
 // CHECK-NEXT:  "name": "j",
 // CHECK-NEXT:  "type": {
 // CHECK-NEXT:   "qualType": "int"
Index: clang/test/AST/ast-dump-if-json.cpp
===
--- clang/test/AST/ast-dump-if-json.cpp
+++ clang/test/AST/ast-dump-if-json.cpp
@@ -709,7 +709,6 @@
 // CHECK-NEXT:"tokLen": 2
 // CHECK-NEXT:   }
 // CHECK-NEXT:  },
-// CHECK-NEXT:  "isUsed": true,
 // CHECK-NEXT:  "name": "i",
 // CHECK-NEXT:  "type": {
 // CHECK-NEXT:   "qualType": "int"
Index: clang/lib/Sema/SemaExprCXX.cpp
===
--- clang/lib/Sema/SemaExprCXX.cpp
+++ clang/lib/Sema/SemaExprCXX.cpp
@@ -3998,6 +3998,11 @@
   ConditionVar, ConditionVar->getType().getNonReferenceType(), VK_LValue,
   ConditionVar->getLocation());
 
+  // Ensure that `-Wunused-variable` will be emitted for condition variables
+  // that are not referenced or used later. e.g.: if (int var = init());
+  ConditionVar->setReferenced(false);
+  ConditionVar->setIsUsed(false);
+
   switch (CK) {
   case