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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
