gribozavr added inline comments.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:491 + ArrayRef<SourceRange> Ranges = None, + ArrayRef<FixItHint> Fixits = None); ---------------- 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> 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
