[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