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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
