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