NoQ marked an inline comment as done. NoQ added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:491 + ArrayRef<SourceRange> Ranges = None, + ArrayRef<FixItHint> Fixits = None); ---------------- gribozavr wrote: > NoQ wrote: > > gribozavr wrote: > > > I'm not sure if this is the right model for fixits. Fixits are usually > > > associated with a message that explains what the fixit does. Only in the > > > unusual case where Clang or ClangTidy is very confident that the fixit is > > > correct, it is attached to the warning. Most commonly, fixits are > > > attached to notes. > > > > > > Also, for IDE support, it would be really nice if we could provide short > > > descriptions of edits themselves (e.g., "replace 'virtual' with > > > 'override") that can be displayed to the user instead of the diff when > > > possible -- right now we don't and tools using ClangTidy have to use a > > > subpar UI because of that. For example, when we show UI for fixing a > > > warning, displaying the complete diff is too much; a concise description > > > would be a lot better. > > > Fixits are usually associated with a message that explains what the fixit > > > does. Only in the unusual case where Clang or ClangTidy is very confident > > > that the fixit is correct, it is attached to the warning. > > > > Wait, you guys already have fixits attached to notes? Then, yeah, i need to > > support this. > > > > > Also, for IDE support, it would be really nice if we could provide short > > > descriptions of edits themselves (e.g., "replace 'virtual' with > > > 'override") that can be displayed to the user instead of the diff when > > > possible -- right now we don't and tools using ClangTidy have to use a > > > subpar UI because of that. > > > > Mmm, interesting. I've seen this IDE of ours autogenerate such messages as > > "replace '`$code_in_removed_range`' with '`$inserted_code`'" (while also > > combining multiple parts of the fixit into a single replacement under the > > hood) and it looked quite bearable most of the time and i silently assumed > > that everybody does the same thing. > > > > I agree that a high-level description of a fixit is nice to have. But given > > that you're attaching fixits to notes, isn't the text of the note text > > itself sufficient? E.g.: > > - warning: variable may be used uninitialized here > > - note: initialize 'x' here to suppress the warning > > - fixit: add ' = 0' after 'x' > > Wait, you guys already have fixits attached to notes? Then, yeah, i need to > > support this. > > Yes, fixits are attached to notes in Clang and ClangTidy. > > > Mmm, interesting. I've seen this IDE of ours autogenerate such messages as > > "replace '$code_in_removed_range' with '$inserted_code'" (while also > > combining multiple parts of the fixit into a single replacement under the > > hood) and it looked quite bearable most of the time and i silently assumed > > that everybody does the same thing. > > We are doing the same thing, however, it only works well when the edit text > is meaningful. For example, a hypothetical "replace const with constexpr" > edit looks readable. However, "insert '(*' and insert ')'" does not read > well, "dereference the pointer" would be better. > > > I agree that a high-level description of a fixit is nice to have. But given > > that you're attaching fixits to notes, isn't the text of the note text > > itself sufficient? E.g.: > > Often yes, however not always. Also, fixits can be attached to warnings > themselves. For example: > > warning: #includes are not sorted according to the style guide > attached fixit: <remove all #includes>, <insert the same #includes in > different order> I see, thanks! It sounds like we might want to extend the `FixItHint` class itself to include an optional note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65182/new/ https://reviews.llvm.org/D65182 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
