aaron.ballman 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;
----------------
xgsa wrote:
> 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
> In spite of the fact, that nolint-usage is technically not a check
That's part of what concerns me about this in terms of discoverability and
usability -- it's not a real check but it sort of behaves like one. Does this
get listed with -list-checks? If the user passes `-nolint-*` on the command
line does it still get disabled?
Also, all of this is predicated by the fact that these nolint warnings are
going to trigger in such a way that sometimes needs to be silenced. What I
think isn't very intuitive is that the recommended way to silence a
nolint-comment-related diagnostic is to use another nolint comment to disable
reporting bugs with nolint comments. However, documentation can help with that,
so perhaps it's not bad.
================
Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:117
+const StringRef ClangDiagnosticsPrefix = "clang-diagnostic-";
+const StringRef NolintCheckName = "readability-nolint-usage";
+enum class NolintCommentType : uint8_t {
----------------
Why "readability"? That module is generally for code readability issues like
redundant code or naming conventions. I think this should probably be in "misc"
or perhaps "bugprone".
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
