[PATCH] D65182: [analyzer] Add fix-it hint support.

Fri Sep 6 06:26:31 PDT 2019

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