[PATCH] D37413: [X86][MS-InlineAsm] Extended support for variables / identifiers on memory / immediate expressions

[coby via Phabricator via cfe-commits]

coby added inline comments.


================
Comment at: lib/Sema/SemaStmtAsm.cpp:617
+    return;
+  } else if (Res->isRValue()) {
+    bool Enum = isa<clang::EnumType>(T) && Res->EvaluateAsRValue(Eval, Context);
----------------
rnk wrote:
> RKSimon wrote:
> > (style) Split these instead of an if-elseif chain
> LLVM suggests early returns: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
> 
> I don't particularly care here, up to you. I personally like to abuse the ability of C++ to return void to write things like this and skip braces, but maybe *I'm* too clever for my own good:
>   if (T->isFunctionType() || T->isDependentType())
>     return Info.setLabel(Res);
>   if (Res->isRValue()) {
>     if (isa<clang::EnumType>(T) && Res->EvaluateAsRValue(Eval, Context))
>       return Info.setEnum(Eval.Val.getInt().getSExtValue());
>     return Info.setLabel(Res);
>   }
brilliant. was holding myself from emitting such **//returns//**, as i had the impression such kind of things are considered taboo. hate redundant bracs. will happily address that.
Anything else?


================
Comment at: lib/Sema/SemaStmtAsm.cpp:620
+    bool Enum = isa<clang::EnumType>(T) && Res->EvaluateAsRValue(Eval, Context);
+    Enum ? Info.setEnum(Eval.Val.getInt().getSExtValue()) : Info.setLabel(Res);
+    return;
----------------
rnk wrote:
> This conditional expression is a bit too cute, I'd rather use a regular if.
ah.. can't blame me for trying :)


Repository:
  rL LLVM

https://reviews.llvm.org/D37413