Avoid assignment in conditional

[urnathan]

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.

[• Quuxplusone]

clang/lib/Sema/SemaDecl.cpp

12234–12239

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]

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–12239

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.

[urnathan]

clang/lib/Sema/SemaDecl.cpp

12234–12239

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.

clang/lib/Sema/SemaDecl.cpp

if (RequireNonAbstractType(VDecl->getLocation(), VDecl->getType(),
                                diag::err_abstract_type_in_decl,
                                AbstractVariableType))
       VDecl->setInvalidDecl();
   }
 
   // If adding the initializer will turn this declaration into a definition,
   // and we already have a definition for this variable, diagnose or otherwise
   // handle the situation.
-  VarDecl *Def;
-  if ((Def = VDecl->getDefinition()) && Def != VDecl &&
+  if (VarDecl *Def = VDecl->getDefinition())
+    if (Def != VDecl &&
         (!VDecl->isStaticDataMember() || VDecl->isOutOfLine()) &&
         !VDecl->isThisDeclarationADemotedDefinition() &&
         checkVarDeclRedefinition(Def, VDecl))
       return;
 
   if (getLangOpts().CPlusPlus) {
     // C++ [class.static.data]p4
     //   If a static data member is of const integral or const
     //   enumeration type, its declaration in the class definition can
     //   specify a constant-initializer which shall be an integral
     //   constant expression (5.19). In that case, the member can appear
     //   in integral constant expressions. The member shall still be
if (!VDecl->isInvalidDecl()) {
 
     Init = Result.getAs<Expr>();
   }
 
   // Check for self-references within variable initializers.
   // Variables declared within a function/method body (except for references)
   // are handled by a dataflow analysis.
   // This is undefined behavior in C++, but valid in C.
-  if (getLangOpts().CPlusPlus) {
+  if (getLangOpts().CPlusPlus)
     if (!VDecl->hasLocalStorage() || VDecl->getType()->isRecordType() ||
-        VDecl->getType()->isReferenceType()) {
+        VDecl->getType()->isReferenceType())
       CheckSelfReference(*this, RealDecl, Init, DirectInit);
-    }
-  }
 
   // If the type changed, it means we had an incomplete type that was
   // completed by the initializer. For example:
   //   int ary[] = { 1, 3, 5 };
   // "ary" transitions from an IncompleteArrayType to a ConstantArrayType.
   if (!VDecl->isInvalidDecl() && (DclT != SavT))
     VDecl->setType(DclT);