xgsa added inline comments.
================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840
+ case NolintCommentType::Nolint:
+ Message = "there is no diagnostics on this line, "
+ "the NOLINT comment is redundant";
+ break;
----------------
aaron.ballman wrote:
> xgsa wrote:
> > aaron.ballman wrote:
> > > xgsa wrote:
> > > > aaron.ballman wrote:
> > > > > I don't think the user is going to care about the distinction between
> > > > > no diagnostics being triggered and the expected diagnostic not being
> > > > > triggered. Also, it's dangerous to claim the lint comment is
> > > > > redundant because it's possible the user has NOLINT(foo, bar) and
> > > > > while foo is not triggered, bar still is. The NOLINT comment itself
> > > > > isn't redundant, it's that the check specified doesn't occur.
> > > > >
> > > > > I would consolidate those scenarios into a single diagnostic:
> > > > > "expected diagnostic '%0' not generated" and "expected diagnostic
> > > > > '%0' not generated for the following line".
> > > > >
> > > > > One concern I have with this functionality is: how should users
> > > > > silence a lint diagnostic that's target sensitive? e.g., a diagnostic
> > > > > that triggers based on the underlying type of size_t or the
> > > > > signedness of plain char. In that case, the diagnostic may trigger
> > > > > for some targets but not others, but on the targets where the
> > > > > diagnostic is not triggered, they now get a diagnostic they cannot
> > > > > silence. There should be a way to silence the "bad NOLINT"
> > > > > diagnostics.
> > > > > I don't think the user is going to care about the distinction between
> > > > > no diagnostics being triggered and the expected diagnostic not being
> > > > > triggered. Also, it's dangerous to claim the lint comment is
> > > > > redundant because it's possible the user has NOLINT(foo, bar) and
> > > > > while foo is not triggered, bar still is. The NOLINT comment itself
> > > > > isn't redundant, it's that the check specified doesn't occur.
> > > > >
> > > > > I would consolidate those scenarios into a single diagnostic:
> > > > > "expected diagnostic '%0' not generated" and "expected diagnostic
> > > > > '%0' not generated for the following line".
> > > >
> > > > This branch of `if (NolintEntry.first.CheckName ==
> > > > NolintCommentsCollector::AnyCheck)` reports only about
> > > > `NOLINT`/`NOLINTNEXTLINE` comments without check list, so I suppose
> > > > it's fair to claim that this comment is redundant (we have already
> > > > checked that no single check reported diagnostics on the line). The
> > > > `else`-branch reports the diagnostics for the definite check in a list
> > > > in case of `NOLINT(foo, bar)` (actually, if neither `foo` nor `bar`
> > > > checks reported diagnostics for the line, there will be a few
> > > > diagnostics from `nolint-usage` - not sure if it's good, but it seems
> > > > acceptable). That is why, I suppose, it is necessary to distinct these
> > > > cases.
> > > >
> > > > > One concern I have with this functionality is: how should users
> > > > > silence a lint diagnostic that's target sensitive? e.g., a diagnostic
> > > > > that triggers based on the underlying type of size_t or the
> > > > > signedness of plain char. In that case, the diagnostic may trigger
> > > > > for some targets but not others, but on the targets where the
> > > > > diagnostic is not triggered, they now get a diagnostic they cannot
> > > > > silence. There should be a way to silence the "bad NOLINT"
> > > > > diagnostics.
> > > >
> > > > There is such mechanism: it is possible to specify `//
> > > > NOLINT(nolint-usage)` or `//NOLINT(check1, check2, nolint-usage) to
> > > > silence the `nolint-usage`-mechanism. Please, see tests for details and
> > > > more examples.
> > > Can you provide an example where this distinction will make a difference
> > > to the user and help clarify a confusing situation? I cannot think of
> > > one, and it would be nice to simplify this code.
> > >
> > > Thank you for the explanation about nolint-usage. This is not terminology
> > > I've seen before -- is this your invention, or is it a documented feature
> > > for NOLINT comments?
> > >> Can you provide an example where this distinction will make a difference
> > >> to the user and help clarify a confusing situation? I cannot think of
> > >> one, and it would be nice to simplify this code.
> >
> > Example for the diagnostics emitted in the `if`-branch:
> > ```
> > class A2 { explicit A2(int i); }; // NOLINT
> > => warning: there is no diagnostics on this line, the NOLINT comment is
> > redundant
> > ```
> > Thus, the whole NOLINT comment should be removed.
> >
> > Example for the diagnostics emitted in the `else`-branch:
> > ```
> > // Case: NO_LINT for the specific check on line with an error on another
> > check.
> > class A4 { A4(int i); }; // NOLINT(misc-unused-parameters,
> > google-explicit-constructor)
> > => warning: there is no diagnostics for the 'misc-unused-parameters' check
> > on this line, the NOLINT comment is redundant
> > ```
> > In this case, only misc-unused-parameters check should be removed from the
> > list, but not the whole NOLINT.
> >
> > Note also that if in the last example there is only misc-unused-parameters
> > in the NOLINT checks list, the diagnostics will be the same. However, I
> > suppose it's acceptable, because even if a user removed only a check name
> > from the check list without removing NOLINT itself, clang-tidy will suggest
> > removing NOLINT itself on the next run.
> >
> >
> > >> Thank you for the explanation about nolint-usage. This is not
> > >> terminology I've seen before -- is this your invention, or is it a
> > >> documented feature for NOLINT comments?
> >
> > It's my invention, I suppose, from the user point of view it's logical to
> > consider this mechanism as a check.
> Thank you for the examples, but I don't think they would add any
> clarification for the user, either. That said, I think we could collapse all
> four diagnostics into: "there are no diagnostics %select{|for '%2'}0 on
> %select{this|the next}1 line" where %0 distinguishes between NOLINT with and
> without an explicit check name, %1 distinguishes between NOLINT and
> NOLINTNEXTLINE, and %2 is the name of the check in question (if any).
>
> >> Thank you for the explanation about nolint-usage. This is not terminology
> >> I've seen before -- is this your invention, or is it a documented feature
> >> for NOLINT comments?
> > It's my invention, I suppose, from the user point of view it's logical to
> > consider this mechanism as a check.
>
> I think we should devise a better way of expressing this unless there's
> industry practice suggesting a specific term or syntax. nolint-usage isn't
> very discoverable. Whatever we wind up with, it should work naturally for
> constructs like your first example. In the first example, diagnosing that
> NOLINT is redundant only makes sense in some circumstances, such as when the
> construct is in a source file that is not compiled with
> google-explicit-constructor enabled. However, if the code is in a shared
> header file where some (but not all) source code is checked with
> google-explicit-constructor enabled, it serves a purpose and removing it
> would be incorrect. The same holds for NOLINT(check).
>
> Ultimately, I think that diagnosing NOLINT on a line that issues no
> diagnostics should not be enabled by default. It's highly likely that the
> NOLINT directive is due to reasons unknowable to the implementation, and
> suggesting the user remove the comment seems unhelpful. My example above is
> one plausible case where NOLINT may serve a purpose. Another example is code
> checked by clang-tidy as well as other tools; the NOLINT may be silencing the
> other tools. I think it should take extra work by the user to enable this
> functionality in a catch-all manner -- either via different syntax or enabled
> only through an explicit flag.
>
> I think that diagnosing NOLINT(check-name) when that diagnostic is not
> triggered is useful by default because the user is explicitly expecting a
> diagnostic by name. However, I think the user needs an almost-trivial way to
> silence the "not triggered" diagnostic on a case-by-case basis. Target
> specific diagnostics as well as shared code are both examples of situations
> where the NOLINT comment may be accurate but for situations unknown to
> clang-tidy. Telling the user to remove the NOLINT comment would be the wrong
> thing to do.
If to look at cpplint, which I suppose, NOLINT syntax was taken from, it also
reports diagnostics for incorrect usage of NOLINT as messages in category
'readability/nolint' [1].
In spite of the fact, that `nolint-usage` is technically not a check, it mimics
the behavior of a simple check: it is only enabled if `nolint-usage` is enabled
via `-check` option (explicitly or via *) or configuration; it reports
diagnostics for this category and allows this diagnostics to be suppressed via
NOLINT. The only missing thing for now is documentation page for the
`nolint-usage` check and I am going to work on this after this patch is
approved. Thus, I don't think that this approach is not discoverable and it
seems more natural than adding a separate command line option and a separate
way of suppressing NOLINT diagnostics.
I agree that this diagnostics will only be useful if NOLINT comments are
carefully maintained exactly for clang-tidy. I don't think that if they are
also added for another tool, the check names or even diagnostics lines will
match. Thus, for these cases the check just shouldn't be enabled in
configuration. The same thing is for headers. I suppose, that typically the
list of checks is the same for the whole project, thus diagnostics for headers
won't be an issue, but if it is not the case, the `nolint-usage` check could
just be disabled for some modules. Anyway, I don't see the way to determine if
a header contains the diagnostics been included from the other source files.
If it is necessary, some configuration options could be added for this check
(e.g. an option to check only `NOLINT` comments in source files, but no
headers; or to check only `NOLINT` comments with/without explicit list of
checks specification).
[1] -
https://github.com/google/styleguide/blob/9663cabfeeea8f1307b1acde59471f74953b8fa9/cpplint/cpplint.py#L577
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D41326
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
