[PATCH] D59135: Add check for matching HeaderFilter before emitting Diagnostic

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:454
+  StringRef FileName(Loc.printToString(Loc.getManager()));
+  if(getHeaderFilter()->match(FileName))
+  {
----------------
thorsten-klein wrote:
> alexfh wrote:
> > aaron.ballman wrote:
> > > Formatting is incorrect here -- you should run the patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting), and elide the braces.
> > I believe, this will break the handling of notes. If the notes attached to the diagnostic are in the "interesting" part of the code (see the checkFilters method below), the whole diagnostic should be treated as such.
> Hello,
> Unfortunately the emitDiagnostic method is called also for files, which should be actually excluded (since the FileName does not match the HeaderFilter regex). This results in diagnostic output, also if there should not be any. For this purpose I added this check here at this point. If the FileName is matching the HeaderFilter regex, then all diagnostic is emitted as before. 
> 
> Did you know about any issue regarding this? If this is not a working solution for you: do you have any other proposal?
> 
> PS: I have also read about this issue in another project: https://fuchsia.googlesource.com/peridot/+/HEAD/docs/lint.md
> --> They say that they disable some clang-tidy checks as workaround
Could you provide a specific example, where you see a problem? Ideally, an isolated set of files + clang-tidy arguments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:454
+  StringRef FileName(Loc.printToString(Loc.getManager()));
+  if(getHeaderFilter()->match(FileName))
+  {
----------------
alexfh wrote:
> thorsten-klein wrote:
> > alexfh wrote:
> > > aaron.ballman wrote:
> > > > Formatting is incorrect here -- you should run the patch through clang-format (https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting), and elide the braces.
> > > I believe, this will break the handling of notes. If the notes attached to the diagnostic are in the "interesting" part of the code (see the checkFilters method below), the whole diagnostic should be treated as such.
> > Hello,
> > Unfortunately the emitDiagnostic method is called also for files, which should be actually excluded (since the FileName does not match the HeaderFilter regex). This results in diagnostic output, also if there should not be any. For this purpose I added this check here at this point. If the FileName is matching the HeaderFilter regex, then all diagnostic is emitted as before. 
> > 
> > Did you know about any issue regarding this? If this is not a working solution for you: do you have any other proposal?
> > 
> > PS: I have also read about this issue in another project: https://fuchsia.googlesource.com/peridot/+/HEAD/docs/lint.md
> > --> They say that they disable some clang-tidy checks as workaround
> Could you provide a specific example, where you see a problem? Ideally, an isolated set of files + clang-tidy arguments.
> Unfortunately the emitDiagnostic method is called also for files, which should be actually excluded (since the FileName does not match the HeaderFilter regex).

As per my initial comment, this is done intentionally to make it possible for a note issued in an "interesting" part of the code to make the whole diagnostic "interesting". Here's a motivating example:

  $ cat a.cc
  #include "b.h"
  $ cat b.h 
  #include "c.h"
  inline void b() { c(/*y=*/42); }
  $ cat c.h
  void c(int x);

If we're only interested in the main file, we don't get any diagnostic:

  $ clang-tidy -checks=-*,bugprone-argument-comment a.cc --
  1 warning generated.
  Suppressed 1 warnings (1 in non-user code).
  Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.

Now if we want diagnostics for `c.h`, we will also get all diagnostics that are (or have notes in) `c.h`:

  $ clang-tidy -checks=-*,bugprone-argument-comment a.cc -header-filter=c.h --
  1 warning generated.
  b.h:2:21: warning: argument name 'y' in comment does not match parameter name 'x' [bugprone-argument-comment]
  inline void b() { c(/*y=*/42); }
                      ^~~~~~
                      /*x=*/
  c.h:1:12: note: 'x' declared here
  void c(int x);
             ^

The fact that we have a note in the "interesting" part of code (be it a header matching -header-filter, or a location matching -line-filter) means that the diagnostic is somehow related to the "interesting" code. It may well be caused by a change in the "interesting" code (e.g. if we changed the parameter name of the function `c` from `y` to `x` in the example above). And for this reason we consider it "interesting".

In some cases this may lead to suboptimal results, I guess, but it would be interesting to see these cases. Could you provide a specific example, where you see a problem? Ideally, an isolated set of files + clang-tidy arguments.


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

https://reviews.llvm.org/D59135