erichkeane added a comment.

1 nit, and 1 trying to see what is going on.  I don't have a good feeling what 
the purpose of this builtin is, nor whether it matches the desire/intent of 
this builtin, I'm hopeful one of the other reviewers has the ability to check 
that.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:2048
+  if (!EltTy->isRealFloatingType()) {
+    S.Diag(Loc, diag::err_builtin_invalid_arg_type)
+        << ArgIndex << /* vector or float ty*/ 5 << ArgTy;
----------------
you can just do "return S.Diag", which always returns 'true'.  This will save a 
line, and the need for curleys.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:2660
+
+    ExprResult Magnitude = UsualUnaryConversions(TheCall->getArg(0));
+    ExprResult Sign = UsualUnaryConversions(TheCall->getArg(1));
----------------
What is the point of the Unary Conversions here?  Its a touch surprising to see 
a builtin do that?  Note that it does quite a bit of FP related conversions, so 
are you sure you want those?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140639/new/

https://reviews.llvm.org/D140639

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to