[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