urusant added a comment.

Thank you for the feedback.

> The patch is missing Sema tests for the attribute (that it only applies to 
> declarations you expect, accepts no args, etc).

There is one test case for that in test/ReturnNonBoolTestCompileTime.cpp. I've 
added another one for attribute accepting no args, so now the last two test 
cases in this file are those you were asking about. Can you think of any other 
cases of invalid attribute usage?

> Have you considered making this a type attribute on the return type of the 
> function rather than a declaration attribute on the function declaration?

No, I hadn't. On a quick look though, I couldn't find a way to simplify my 
solution using this idea, because as far as I understand, the type attribute 
isn't inherited, so, for example, if I have something like `int r = 
X509_verify_cert(...)` and the function `X509_verify_cert` has a return type 
with attribute, `r` won't have the attribute. If that is correct, we still need 
to backtrace the value to the function declaration. Is there something I am 

Comment at: include/clang/Basic/Attr.td:1138
@@ +1137,3 @@
+def WarnImpcastToBool : InheritableAttr {
+  let Spellings = [GCC<"warn_impcast_to_bool">];
+  let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
aaron.ballman wrote:
> This should not use a GCC spelling because it's not an attribute that GCC 
> supports. You should probably use GNU instead, since I suspect this attribute 
> will be useful in C as well as C++.
Yeah, makes sense.

Comment at: include/clang/Basic/Attr.td:1140
@@ +1139,3 @@
+  let Subjects = SubjectList<[ObjCMethod, Function], WarnDiag,
+                             "ExpectedFunctionOrMethod">;
+  let Documentation = [WarnImpcastToBoolDocs];
aaron.ballman wrote:
> No need to specify the WarnDiag or ExpectedFunctionOrMethod arguments; they 
> will be handled automatically.
I didn't know that, thanks.

Comment at: include/clang/Basic/AttrDocs.td:2055
@@ -2054,1 +2054,3 @@
+def WarnImpcastToBoolDocs : Documentation {
+  let Category = DocCatFunction;
zaks.anna wrote:
> You probably need to "propose" the attribute to the clang community. I'd send 
> an email to the cfe-dev as it might not have enough attention if it's just 
> the patch.  
OK, will do.

Comment at: include/clang/Basic/AttrDocs.td:2058
@@ +2057,3 @@
+  let Content = [{
+    The ``warn_impcast_to_bool`` attribute is used to indicate that the return 
value of a function with integral return type cannot be used as a boolean 
value. For example, if a function returns -1 if it couldn't efficiently read 
the data, 0 if the data is invalid and 1 for success, it might be dangerous to 
implicitly cast the return value to bool, e.g. to indicate success. Therefore, 
it is a good idea to trigger a warning about such cases. However, in case a 
programmer uses an explicit cast to bool, that probably means that he knows 
what he is doing, therefore a warning should be triggered only for implicit 
aaron.ballman wrote:
> You should manually wrap this to roughly the 80 col limit.
> Instead of "he", can you use "they" please?
OK, I did that. However, 80 col limit in this case feels a bit inconsistent 
with the rest of the file to me because most of other similar descriptions 
don't follow it.

Comment at: include/clang/Basic/DiagnosticGroups.td:57
@@ -56,2 +56,3 @@
 def DoublePromotion : DiagGroup<"double-promotion">;
+def UnsafeBoolConversion : DiagGroup<"unsafe-bool-conversion">;
 def EnumTooLarge : DiagGroup<"enum-too-large">;
aaron.ballman wrote:
> I'm not certain this requires its own diagnostic group. This can probably be 
> handled under `BoolConversion`

Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2259
@@ +2258,3 @@
+def warn_attribute_return_int_only : Warning<
+  "%0 attribute only applies to return values that are integers">,
+  InGroup<IgnoredAttributes>;
aaron.ballman wrote:
> How about: ...only applies to integer return types?
Yeah, that sounds better.

Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2883
@@ +2882,3 @@
+  "implicit conversion turns non-bool into bool: %0 to %1">,
+  InGroup<UnsafeBoolConversion>, DefaultIgnore;
aaron.ballman wrote:
> I don't think this should be a DefaultIgnore diagnostic -- if the user wrote 
> the attribute, they should get the diagnostic when appropriate.
Makes sense.

Comment at: lib/Sema/SemaChecking.cpp:8262
@@ +8261,3 @@
+    /// e.g. (x ? f : g)(y)
+    if (isa<CallExpr>(E)) {
+      FunctionDecl* fn = cast<CallExpr>(E)->getDirectCallee();
aaron.ballman wrote:
> Should use `if (const auto *CE = dyn_cast<CallExpr>(E)) {`

Comment at: lib/Sema/SemaChecking.cpp:8263-8264
@@ +8262,4 @@
+    if (isa<CallExpr>(E)) {
+      FunctionDecl* fn = cast<CallExpr>(E)->getDirectCallee();
+      if (!fn)
+        return;
aaron.ballman wrote:
> Then you can do `if (const auto *Fn = CE->getDirectCallee()) {`

Comment at: lib/Sema/SemaChecking.cpp:8269
@@ +8268,3 @@
+        S.Diag(fn->getLocation(), diag::note_entity_declared_at)
+                        << fn->getDeclName();
+        return;
aaron.ballman wrote:
> You can pass in `fn` directly, the diagnostics engine will properly get the 
> name out of it because it's derived from `NamedDecl`.
Thanks, didn't notice that.

Comment at: lib/Sema/SemaDeclAttr.cpp:1316
@@ +1315,3 @@
+    S.Diag(Attr.getLoc(), diag::warn_attribute_return_int_only)
+      <<Attr.getName() << SourceRange() << SR;
+    return;
aaron.ballman wrote:
> Formatting seems off -- you should run the patch through clang-format. Also, 
> why are you passing an empty `SourceRange`?
Ok, I ran clang-format.
Good spot, it seems that I don't need that `SourceRange`.

Comment at: test/ReturnNonBoolTest.c:74
@@ +73,3 @@
+  // no good way to get those from the program state.
+  if (restricted_wrap(2)) // expected-warning{{implicit cast to bool is 
dangerous for this value}}
+    return;
zaks.anna wrote:
> I do not understand why this is a false positive. In restricted_wrap, r can 
> be any value. You only return '0' if r is '-1', but it could be '-2' or 
> '100', which are also not bool and this values would just get returned.
> You should be able to query the state to check if a value is a zero or one 
> using code like this from CStringChecker.cpp:
> "
>   SValBuilder &svalBuilder = C.getSValBuilder();
>   DefinedOrUnknownSVal zero = svalBuilder.makeZeroVal(Ty);
>   return state->assume(svalBuilder.evalEQ(state, *val, zero))
> "
I have replaced this test case with another one that illustrates the problem I 
am referring to clearer. Ideally it would be great to have some indicator to 
tell the StaticAnalyzer that we have handled all the dangerous return values, 
and from this point it is safe to use it as a boolean. You can use explicit 
cast to bool or `rc != 0` every time you want to use it, but it is not very 
convenient. Do you have any suggestions on this matter?

As for your proposal, it is not very difficult to add, however, it is not very 
likely to be useful in real codebases for the same reason as in the testcase. 
Do you still think it should be added?

Comment at: test/ReturnNonBoolTestCompileTime.cpp:40
@@ +39,1 @@
+double InvalidAttributeUsage() RETURNS_NON_BOOL; // 
expected-warning{{'warn_impcast_to_bool' attribute only applies to return 
values that are integers}}
\ No newline at end of file

aaron.ballman wrote:
> Can you end the file with a newline?



cfe-commits mailing list

Reply via email to