Page MenuHomePhabricator

Avoid assignment in conditional
ClosedPublic

Authored by urnathan on Jun 10 2021, 8:44 AM.

Details

Summary

I noticed we had:
T v;
if ((v = fn()) && ...)
which is somewhat poor, particularly as 'v' is only needed inside that if. This patch changes to use 'if (T v = fn()) if (...'. IIUC that doesn't need braces (there's a case at 12442 like that). A nearby nested if does use braces though, which this patch also removes.

Diff Detail

Event Timeline

urnathan requested review of this revision.Jun 10 2021, 8:44 AM
urnathan created this revision.
Quuxplusone added inline comments.Jun 10 2021, 8:50 AM
clang/lib/Sema/SemaDecl.cpp
12234–12240

FWIW, in terms of code-understandability, I would even rather see

VarDecl *Def = VDecl->getDefinition();
if (Def != nullptr && Def != VDecl &&
    [...]

although that does let Def's scope leak out into the subsequent lines. Anyway, I don't care much. :)

aaron.ballman accepted this revision.Jun 10 2021, 12:20 PM

Giving this a LG because the code is fine and I don't have strong opinions. If you feel like resolving it the way @Quuxplusone suggests, that's also fine by me.

clang/lib/Sema/SemaDecl.cpp
12234–12240

That's usually my preference as well. If we're truly worried about the scope of the variable, I'd even prefer:

{
  VarDecl *Def = VDecl->getDefinition();
  if (Def != nullptr && Def != VDecl &&
      [...]
}

but honestly, I'm not convinced the scope issue is worth that level of extra indentation.

This revision is now accepted and ready to land.Jun 10 2021, 12:20 PM
urnathan added inline comments.Jun 10 2021, 12:47 PM
clang/lib/Sema/SemaDecl.cpp
12234–12240

That was what my original thought was ... 'wait, I can just RAII it right there', but then we do have if (decl-stmt) these days, and make use of it, particularly in the middle of an if nest. As I mentioned, a little further down we have:

if (auto *E = dyn_cast<ExprWithCleanups>(Init))
  if (auto *BE = dyn_cast<BlockExpr>(E->getSubExpr()->IgnoreParens()))
    if (VDecl->hasLocalStorage())
      BE->getBlockDecl()->setCanAvoidCopyToHeap();

so, as you don't seem overly desirous of not doing what I did, I'll do what I did.
thanks for the comments.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2021, 3:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript