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
