lebedev.ri added a comment.

The checking by itself looks sane-ish, but i don't have any reasonable 
knowledge in this area.



================
Comment at: lib/Sema/SemaChecking.cpp:925
+  case Builtin::BI__builtin_signbitl:
     if (SemaBuiltinFPClassification(TheCall, 1))
       return ExprError();
----------------
The name of the function is unfortunate given that you call it for 
`__builtin_signbit(int)`.


================
Comment at: test/Sema/builtins.c:260
+  (void)__builtin_signbit(1.0, 2.0, 3.0); // expected-error{{too many 
arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbit(1); // expected-error {{floating point 
classification requires argument of floating point type (passed in 'int')}}
+
----------------
What about
```
(void)__builtin_signbit(1.0);
```


================
Comment at: test/Sema/builtins.c:260
+  (void)__builtin_signbit(1.0, 2.0, 3.0); // expected-error{{too many 
arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbit(1); // expected-error {{floating point 
classification requires argument of floating point type (passed in 'int')}}
+
----------------
lebedev.ri wrote:
> What about
> ```
> (void)__builtin_signbit(1.0);
> ```
Hm, is `__builtin_signbit()` taking an integer, or float?
If integer, the comment about `floating point classification` is slightly 
misleading.



================
Comment at: test/Sema/builtins.c:264
+  (void)__builtin_signbitf(1.0, 2.0, 3.0); // expected-error{{too many 
arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbitf(1);
+
----------------
`1` will be promoted to float?
I'd add one more test with native float:
```
(void)__builtin_signbit(1.0);
```


================
Comment at: test/Sema/builtins.c:268
+  (void)__builtin_signbitl(1.0, 2.0, 3.0); // expected-error{{too many 
arguments to function call, expected 1, have 3}}
+  (void)__builtin_signbitl(1);
+}
----------------
Same, would be good to see `__builtin_signbitf(1.0);`, 
`__builtin_signbitf(1.0L);`.


Repository:
  rC Clang

https://reviews.llvm.org/D47435



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

Reply via email to