steveire added a comment.
In D96082#2545807 <https://reviews.llvm.org/D96082#2545807>, @LukasHanel wrote:
> 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
I agree with @njames93 that this check is dangerous. Even if you extended it to
port callExprs, that would only work in translation units which can see the
definition.
I have made checks to do that kind of thing (port from raw pointers to
`unique_ptr`), but it involves running a check to generate a list of functions
that will be ported in one step and doing the actual change (including
callExprs across all TUs) in a second step.
A tool like this would have to something similar to be useful, but I don't
think such a check should be upstream at this point. We don't have good
infrastructure to record the signatures that will be changed. What I did was
create/touch files to the filesystem named as function signatures in the first
step, and in the second step read the list of files to know what signatures
should be ported. Using the filesystem meant uniqueness and that a
concurrently-running instances didn't encounter syncronization issues. Perhaps
something like that could be made generic and upstreamed to support this kind
of case.
In addition to that practical problem, there are some functions which
deliberately always return the same value and you don't have a way to
distinguish them other than by adding an option to ignore them. It might make
the checker a bit of a burden. The kinds of functions I'm referring to include
functions which give a name to an otherwise-magic number (`int sentinalValue()
{ return 0; }`), virtual methods (I see you already handle those), Visitors,
dummy implementations of APIs which may not be implemented on a platform, CRTP
interfaces, there may be others.
Am I missing something?
> - 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.
Does this mean that you didn't try to run this tool on a real codebase? (ie
your company code is already changed and doesn't need it anymore). You may run
into the cross-TU issue if you run it on a codebase.
> Clang-tidy is a really easy environment to write such checkers, Kudos to all
> the contributors and maintainers.
Glad it's useful to you.
> 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 think you might be right. Do you have a list of such checks? Maybe they
should be removed.
> I did not find any other checker that changed the function signature, is
> there any guidance against this?
I don't think there are any because they don't work across TUs. If we don't
have guidance on this, we should probably create some. My position is that the
guidance should say that we shouldn't have such checks until we have the
required infrastructure to implement them correctly.
>> 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
> I 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).
Indeed, this doesn't seem to be an issue. If the function returns a constant,
then the behavior at the call site is known and can be simplified to operate
the same way without checking the return value.
> - 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.
Yes.
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
