[PATCH] D123352: [analyzer] Add FixItHint to `nullability.NullReturnedFromNonnull` and `nullability.NullableReturnedFromNonnull`

Fri Apr 8 08:31:02 PDT 2022

MosheBerman added a comment.

In D123352#3438475 <https://reviews.llvm.org/D123352#3438475>, @steakhal wrote:

> You have a single bool property, yet you allow to enable/disable this by each sub-checker. It feels like the last checker in the registration process will rule them all.
>
> That being said, in the fixit creation scope, you check for both this flag and the presence of the fixit location - which you only set if the flag is active.
>
> IMO you should have this flag per-subchecker, and check for the presence of that and either pass the fixit location if you actually need to insert the fixit or pass it unconditionally and emit the fixit only if the flag of the given sub-checker is set.

That’s a helpful observation. Besides for the code hygiene, I am completely new to LLVM and C++, so I wasn’t aware that the flags will override. I’ll think more about this and address this feedback in an update to this patch.

> Also, make sure the tests pass.

Yep - the reason I posted this diff in the current state was because I’m unsure why the fixit isn’t appearing in the test output. I posted about it [on Discourse][1].

Is there something I need to do besides for attaching the hint to the bug report?

I also found [a really old post][2] where @ted_kremenek says that `FixItHint`s were - at the time - not implemented on `BugReporter`. I don’t know if that’s changed since then, or if the approach has been to use clang-tidy exclusive to supporting fixits in checkers.

Do you have any insight?

Thanks so much for taking the time to look at this and provide thoughtful feedback!

[1]: https://discourse.llvm.org/t/help-testing-fix-it-hints-in-existing-checker/61565
[2]: https://discourse.llvm.org/t/analysis-vs-fixit/14882/2

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123352/new/

https://reviews.llvm.org/D123352