[PATCH] D109210: [clang-tidy] Attach fixit to warning, not note, in add_new_check.py example

Tue Sep 14 12:13:33 PDT 2021

mattbeardsley added a comment.

Thanks for your reply and sorry about my very sluggish reply!
I am looking into updating the docs as you suggested, and that got me looking at this doc page <https://clang.llvm.org/extra/clang-tidy/Contributing.html#writing-a-clang-tidy-check>. Interestingly, that doc page's version of the add_new_check.py template doesn't have the `Note` diagnostic. So that got me digging into the history.
It seems like 2 commits are at odds with each other about what the right thing to do is with `FixItHint`s w.r.t. `Note` vs `Warning` diagnostics.

1. This commit <https://github.com/llvm/llvm-project/commit/f2879d8a4877eafcdb12c852030746d175f8abbd> says that "fix descriptions and fixes are emitted via diagnostic::Note (rather than attaching the main warning diagnostic)."  - And note how that commit changes add_new_check.py - adding the `Note` diagnostic!
2. This commit <https://reviews.llvm.org/rGabbe9e227ed31e5dde9bb7567bb9f0dd047689c6> seems to suggest that Note diagnostics should //not// be used to provide the "standard" fix (and generally enforces that by `--fix-notes` being off by default)

It looks like nearly all existing check implementations disagree with the former, and agree with the latter.
Those two commits appear to be at odds with each other on the semantics of `Note` diagnostics - any opinions on how to reconcile that apparent disagreement?

My (limited) perspective is:
Since `--fix-notes` is off by default, it seems like `FixIt`s on `Note` diagnostics are pretty well enforced to be used for secondary/uncertain suggestions, while `FixIt`s on `Warning` diagnostics are for the primary fix(es). 
That seems to rule out accepting the guidance of first commit w.r.t. attaching fixes to `Note` diagnostics given the `--fix-notes` flag imposes that such note-fixes are implicitly secondary to warning-fixes.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109210