aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:24
+ "ERR_CAST", "PTR_ERR_OR_ZERO"));
+ auto NonCheckingStmts = stmt(anyOf(compoundStmt(), labelStmt()));
+ Finder->addMatcher(
----------------
tmroeder wrote:
> lebedev.ri wrote:
> > Are there any rules what kernel considers to be checking and not checking?
> > This i think e.g. will accept cast-to-void, assignment to a variable and
> > then ignoring it, etc.
> >
> Yes, this check is extremely simplistic. My understanding of clang-tidy
> checks was that the most value comes from them catching obviously wrong
> behavior and not having any false positives. I didn't think they were
> supposed to catch all the ways in which something could be wrong.
>
> But I've never written a clang-tidy check before. What is their expected
> purpose?
We are more forgiving of false positives in clang-tidy than we are in the
compiler proper, so we're okay with checks being more chatty, within reason.
Obviously, the less false positives (and false negatives), the better because
it's more useful for the user.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:39
+ if (MatchedCallExpr &&
+ MatchedCallExpr->hasUnusedResultAttr(*Result.Context)) {
+ diag(MatchedCallExpr->getExprLoc(), "Unused result from error function %0")
----------------
tmroeder wrote:
> aaron.ballman wrote:
> > Is the presence of the attribute actually important, or is it simply
> > expected that the declarations will have the attribute and so this check is
> > benign?
> The latter. But I think I could remove it; it seems unlikely that the
> attribute will be removed from these functions any time, though it could be
> disabled. It gets set in include/linux/compiler-gcc.h because clang sets the
> macros __GNUC__ and __GNUC_MINOR__ and __GNUC_PATCHLEVEL__ greater than 3, 4,
> and 0, respectively.
I'd probably remove it -- the check is useful even if the function is not
marked with the attribute, but if the function is expected to be marked with
the attribute in all circumstances anyway, then this is just doing needless
work.
================
Comment at: clang-tools-extra/clang-tidy/linuxkernel/MustCheckErrsCheck.cpp:51
+ }
+}
+
----------------
tmroeder wrote:
> aaron.ballman wrote:
> > Is there a way for the user to explicitly disable this warning, or is there
> > no such thing as a false positive as far as the kernel is concerned?
> >
> > Should the diagnostic also produce a fix-it, or is there not a sufficiently
> > common replacement pattern to be used?
> The basic case (are these error functions being used in any way at all?) is
> an error and should be fixed. As noted in response to a comment above, the
> check in that case is so naive that anything it catches is wrong.
>
> With respect to not using the result from functions that return this value, I
> think the same argument applies: if code calls a function that returns an
> error like this and literally ignores the result, then clang-tidy should flag
> it. However, I don't know of a tool that currently checks the kernel for this
> kind of transitive property with respect to these functions, so it's possible
> that there are errors like that in the kernel (or idioms that I don't know
> about).
>
> I thought that the way that you turn off clang-tidy checks is by specifying
> them at the command line with a minus sign in front of them:
> -checks=-linuxkernel-must-check-errs.
>
> Or do you mean locally turning it off? In that case, you can just use the
> result of the function in any trivial way, like casting it to void.
>
> With respect to the fixit; I thought about that, but I'm not sure I know what
> the right fixit should be. I'd like to leave it without a fixit for now and
> come back to it later if it becomes clear what to do here.
> The basic case (are these error functions being used in any way at all?) is
> an error and should be fixed. As noted in response to a comment above, the
> check in that case is so naive that anything it catches is wrong.
That sounds great to me.
> Or do you mean locally turning it off? In that case, you can just use the
> result of the function in any trivial way, like casting it to void.
That's mostly what I was trying to understand. We usually want checks to have
some way to be silenced locally (cast to void, NOLINT comments, whatever makes
sense for the construct), though it sounds like your situation probably doesn't
need it because every instance caught is truly a bug.
> With respect to the fixit; I thought about that, but I'm not sure I know what
> the right fixit should be. I'd like to leave it without a fixit for now and
> come back to it later if it becomes clear what to do here.
Sounds fine to me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D59963/new/
https://reviews.llvm.org/D59963
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
