LukasHanel added a comment. Hi, thanks for discussing my proposal! Although I think it can stand as is, I was looking for feedback:
- Is the name good? - Is the `readability` group good? Or better in `misc`? - too slow, too fast? - More precision required? - Usefulness of the fix-it's - Should I add options? Anyhow, last year I was refactoring our companies code base and often manually searched for functions that would be identified by such checker. And as you say, I did not implement the corner cases yet. Clang-tidy is a really easy environment to write such checkers, Kudos to all the contributors and maintainers. On your comments: In D96082#2544836 <https://reviews.llvm.org/D96082#2544836>, @njames93 wrote: > This check, more specifically the fixes, seem quite dangerous. Changing > function signatures is highly likely to cause compilation issues. This seems to be a general issue with clang-tidy's fixes. It seems some fixes are more of a gimmick and shouldn't be used without supervision. I did not find any other checker that changed the function signature, is there any guidance against this? > For a starters this check doesn't appear to examine call sites of the > function to ensure they don't use the return value. I have considered this, but it would double the complexity of the checker, so did not work on it yet. Yes, it is a good safety net. However, it could also hide two issues: 1. Inconsistent use (or not use) of the return value - it is actually a sign of the return value being useless (Note: this is a common checker for commercial static analyzers). 2. Propagation of the useless return value, let's say `b` checks useless return value of `a`, and returns it, but `c` does not check return value of `b`. int a() { return 0; } // "useless" int b() { return a(); } // "checked" int c() { b(); // not checked // or even int ret = b(); // "checked" if (ret) { printf("something went wrong"); // or maybe the return value was useless in the first place } //... } Regarding the propagation, anyhow, such refactoring needs to be done in steps: you fix one function, then the linter will highlight the next function. > There's also the case where the function is used as a callback, this can't > change the signature as the callback type would then mismatch. Yes, we had this case as well. I wonder how I could detect this. I guess I need the call sites. 1. List Item 2. Or a configuration for this checker of functions to ignore. 3. Or somehow annotate functions to ignore this checker. > From these 2 issues, this shouldn't trigger on functions with external > linkage. From my experience, this is actually more a problem for external functions. Static functions tend to less have such issues because the deveoloper can more easily verify the callers. > There's the case where the return value is based on some preprocessor > constant. You are showing how it handles NULL and EXIT_SUCCESS, however what > if the macro is called BUILT_WITH_FEATURE_X, that's likely a build setting > and shouldn't be triggered upon. Obviously there's no hard and fast way to > determine what's a dependent macro and what isn't, so either never trigger on > returns where the value is a macro, or have a list of allowed macros, > defaulted to containing the likes of NULL. Curious case of the command-line defined return-value macro. :) When playing with this part, I found that clang-tidy sees the code after preprocessing. So I never saw that I was handling NULL or EXIT_SUCCESS, I only saw `0`. Clang-tidy must know about build-time macros from the compile_commands.json data base, otherwise it would scream. Hmm, I start to see what you mean. > With all being said, I feel that while the diagnosing of these functions can > have some value, an automated fix probably isn't a good idea. Thanks. Will the CI run my checker on the llvm code base? I found several issues locally, I will figure out how to best share those findings. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96082/new/ https://reviews.llvm.org/D96082 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
