[PATCH] D24507: Add attribute for return values that shouldn't be cast to bool

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this check! A few comments:

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

Have you considered making this a type attribute on the return type of the function rather than a declaration attribute on the function declaration? Right now, the diagnostic you receive on a conversion may be spatially separated from where the user called the function (including crossing translation unit boundaries, which the static analyzer doesn't currently handle). By putting the attribute on the type, it carries more obvious semantic meaning and means the check can happen entirely in the frontend (no static analyzer required). For instance: `typedef __attribute__((warn_impcast_to_bool)) IntNotABool;` I'm not certain if this is a better design or not, but I am wondering if it was something you had considered.


================
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,
----------------
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++.

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

================
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 casts.
+
----------------
You should manually wrap this to roughly the 80 col limit.

Instead of "he", can you use "they" please?

================
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">;
----------------
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>;
----------------
How about: ...only applies to integer return types?

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

================
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();
----------------
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;
----------------
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;
----------------
You can pass in `fn` directly, the diagnostics engine will properly get the name out of it because it's derived from `NamedDecl`.

================
Comment at: lib/Sema/SemaDeclAttr.cpp:1316
@@ +1315,3 @@
+    S.Diag(Attr.getLoc(), diag::warn_attribute_return_int_only)
+      <<Attr.getName() << SourceRange() << SR;
+    return;
----------------
Formatting seems off -- you should run the patch through clang-format. Also, why are you passing an empty `SourceRange`?

================
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

----------------
Can you end the file with a newline?


Repository:
  rL LLVM

https://reviews.llvm.org/D24507