Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
Oh, yeah, that should probably be clamped. I didn’t have a specific use case in mind, but in general I find that tools that emit “richer” information can be used for more things, hence returning the count, and not some fixed value. On Sat, Jun 13, 2020 at 12:58 AM Nathan James via Phabricator < revi...@reviews.llvm.org> wrote: > njames93 added inline comments. > Herald added a subscriber: arphaman. > > > > Comment at: clang-tidy/tool/ClangTidyMain.cpp:362 > + << Plural << "\n"; > +return WErrorCount; > + } > > Was there any specific reason for returning the error count instead of > returning 1. It results in undefined behaviour on POSIX shells if a value > is returned outside the range of 0<=n<=255. See > https://bugs.llvm.org/show_bug.cgi?id=46305 > > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D15528/new/ > > https://reviews.llvm.org/D15528 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs abandoned this revision. jroelofs added a comment. Hmm. There's no "close revision" button. Abandoning it in lieu of closing it. http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
alexfh added a subscriber: jroelofs. alexfh added a comment. In http://reviews.llvm.org/D15528#326031, @jroelofs wrote: > Hmm. There's no "close revision" button. Abandoning it in lieu of closing it. Weird. That's probably because I've not marked it "Accepted". Now I tried a few other actions on the revision and it now shows as authored by myself. Really weird. Anyway, thank you for working on this! http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs updated this revision to Diff 44650.
jroelofs added a comment.
Added docs.
http://reviews.llvm.org/D15528
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyOptions.cpp
clang-tidy/ClangTidyOptions.h
clang-tidy/tool/ClangTidyMain.cpp
docs/clang-tidy/index.rst
test/clang-tidy/werrors-plural.cpp
Index: test/clang-tidy/werrors-plural.cpp
===
--- test/clang-tidy/werrors-plural.cpp
+++ test/clang-tidy/werrors-plural.cpp
@@ -0,0 +1,15 @@
+// RUN: clang-tidy %s -checks='-*,llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WARN
+// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment' -warnings-as-errors='llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WERR
+
+namespace j {
+}
+// CHECK-WARN: warning: namespace 'j' not terminated with a closing comment [llvm-namespace-comment]
+// CHECK-WERR: error: namespace 'j' not terminated with a closing comment [llvm-namespace-comment,-warnings-as-errors]
+
+namespace k {
+}
+// CHECK-WARN: warning: namespace 'k' not terminated with a closing comment [llvm-namespace-comment]
+// CHECK-WERR: error: namespace 'k' not terminated with a closing comment [llvm-namespace-comment,-warnings-as-errors]
+
+// CHECK-WARN-NOT: treated as
+// CHECK-WERR: 2 warnings treated as errors
Index: docs/clang-tidy/index.rst
===
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -68,7 +68,9 @@
diagnostics are displayed by clang-tidy and can be filtered out using
``-checks=`` option. However, the ``-checks=`` option does not affect
compilation arguments, so it can not turn on Clang warnings which are not
-already turned on in build configuration.
+already turned on in build configuration. The ``-warnings-as-errors=`` option
+upgrades any warnings emitted under the ``-checks=`` flag to errors (but it
+does not enable any checks itself).
Clang diagnostics have check names starting with ``clang-diagnostic-``.
Diagnostics which have a corresponding warning option, are named
@@ -150,6 +152,7 @@
-checks=* to list all available checks.
-p=- Build path
-system-headers- Display the errors from system headers.
+-warnings-as-errors= - Upgrades warnings to errors. Same format as '-checks'.
-p is used to read a compile command database.
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -64,6 +64,11 @@
cl::init(""), cl::cat(ClangTidyCategory));
static cl::opt
+WarningsAsErrors("warnings-as-errors", cl::desc("Upgrades warnings to errors. "
+"Same format as '-checks'."),
+ cl::init(""), cl::cat(ClangTidyCategory));
+
+static cl::opt
HeaderFilter("header-filter",
cl::desc("Regular expression matching the names of the\n"
"headers to output diagnostics from. Diagnostics\n"
@@ -227,6 +232,7 @@
ClangTidyOptions DefaultOptions;
DefaultOptions.Checks = DefaultChecks;
+ DefaultOptions.WarningsAsErrors = "";
DefaultOptions.HeaderFilterRegex = HeaderFilter;
DefaultOptions.SystemHeaders = SystemHeaders;
DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
@@ -238,6 +244,8 @@
ClangTidyOptions OverrideOptions;
if (Checks.getNumOccurrences() > 0)
OverrideOptions.Checks = Checks;
+ if (WarningsAsErrors.getNumOccurrences() > 0)
+OverrideOptions.WarningsAsErrors = WarningsAsErrors;
if (HeaderFilter.getNumOccurrences() > 0)
OverrideOptions.HeaderFilterRegex = HeaderFilter;
if (SystemHeaders.getNumOccurrences() > 0)
@@ -322,8 +330,10 @@
const bool DisableFixes = Fix && FoundErrors && !FixErrors;
+ unsigned WErrorCount = 0;
+
// -fix-errors implies -fix.
- handleErrors(Errors, (FixErrors || Fix) && !DisableFixes);
+ handleErrors(Errors, (FixErrors || Fix) && !DisableFixes, WErrorCount);
if (!ExportFixes.empty() && !Errors.empty()) {
std::error_code EC;
@@ -344,6 +354,13 @@
if (EnableCheckProfile)
printProfileData(Profile, llvm::errs());
+ if (WErrorCount) {
+StringRef Plural = WErrorCount == 1 ? "" : "s";
+llvm::errs() << WErrorCount << " warning" << Plural
+ << " treated as error" << Plural << "\n";
+return WErrorCount;
+ }
+
return 0;
}
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -62,6 +62,9 @@
/// \brief Checks filter.
llvm::Optional Checks;
+ /// \brief WarningsAsErrors filter.
+ llvm::Optional WarningsAsErrors;
+
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs updated this revision to Diff 44648.
jroelofs added a comment.
Address review comments.
http://reviews.llvm.org/D15528
Files:
clang-tidy/ClangTidy.cpp
clang-tidy/ClangTidy.h
clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tidy/ClangTidyOptions.cpp
clang-tidy/ClangTidyOptions.h
clang-tidy/tool/ClangTidyMain.cpp
test/clang-tidy/werrors-plural.cpp
Index: test/clang-tidy/werrors-plural.cpp
===
--- test/clang-tidy/werrors-plural.cpp
+++ test/clang-tidy/werrors-plural.cpp
@@ -0,0 +1,15 @@
+// RUN: clang-tidy %s -checks='-*,llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WARN
+// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment' -warnings-as-errors='llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WERR
+
+namespace j {
+}
+// CHECK-WARN: warning: namespace 'j' not terminated with a closing comment [llvm-namespace-comment]
+// CHECK-WERR: error: namespace 'j' not terminated with a closing comment [llvm-namespace-comment,-warnings-as-errors]
+
+namespace k {
+}
+// CHECK-WARN: warning: namespace 'k' not terminated with a closing comment [llvm-namespace-comment]
+// CHECK-WERR: error: namespace 'k' not terminated with a closing comment [llvm-namespace-comment,-warnings-as-errors]
+
+// CHECK-WARN-NOT: treated as
+// CHECK-WERR: 2 warnings treated as errors
Index: clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tidy/tool/ClangTidyMain.cpp
@@ -64,6 +64,11 @@
cl::init(""), cl::cat(ClangTidyCategory));
static cl::opt
+WarningsAsErrors("warnings-as-errors", cl::desc("Upgrades warnings to errors. "
+"Same format as '-checks'."),
+ cl::init(""), cl::cat(ClangTidyCategory));
+
+static cl::opt
HeaderFilter("header-filter",
cl::desc("Regular expression matching the names of the\n"
"headers to output diagnostics from. Diagnostics\n"
@@ -227,6 +232,7 @@
ClangTidyOptions DefaultOptions;
DefaultOptions.Checks = DefaultChecks;
+ DefaultOptions.WarningsAsErrors = "";
DefaultOptions.HeaderFilterRegex = HeaderFilter;
DefaultOptions.SystemHeaders = SystemHeaders;
DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;
@@ -238,6 +244,8 @@
ClangTidyOptions OverrideOptions;
if (Checks.getNumOccurrences() > 0)
OverrideOptions.Checks = Checks;
+ if (WarningsAsErrors.getNumOccurrences() > 0)
+OverrideOptions.WarningsAsErrors = WarningsAsErrors;
if (HeaderFilter.getNumOccurrences() > 0)
OverrideOptions.HeaderFilterRegex = HeaderFilter;
if (SystemHeaders.getNumOccurrences() > 0)
@@ -322,8 +330,10 @@
const bool DisableFixes = Fix && FoundErrors && !FixErrors;
+ unsigned WErrorCount = 0;
+
// -fix-errors implies -fix.
- handleErrors(Errors, (FixErrors || Fix) && !DisableFixes);
+ handleErrors(Errors, (FixErrors || Fix) && !DisableFixes, WErrorCount);
if (!ExportFixes.empty() && !Errors.empty()) {
std::error_code EC;
@@ -344,6 +354,13 @@
if (EnableCheckProfile)
printProfileData(Profile, llvm::errs());
+ if (WErrorCount) {
+StringRef Plural = WErrorCount == 1 ? "" : "s";
+llvm::errs() << WErrorCount << " warning" << Plural
+ << " treated as error" << Plural << "\n";
+return WErrorCount;
+ }
+
return 0;
}
Index: clang-tidy/ClangTidyOptions.h
===
--- clang-tidy/ClangTidyOptions.h
+++ clang-tidy/ClangTidyOptions.h
@@ -62,6 +62,9 @@
/// \brief Checks filter.
llvm::Optional Checks;
+ /// \brief WarningsAsErrors filter.
+ llvm::Optional WarningsAsErrors;
+
/// \brief Output warnings from headers matching this filter. Warnings from
/// main files will always be displayed.
llvm::Optional HeaderFilterRegex;
Index: clang-tidy/ClangTidyOptions.cpp
===
--- clang-tidy/ClangTidyOptions.cpp
+++ clang-tidy/ClangTidyOptions.cpp
@@ -85,6 +85,7 @@
MappingNormalization NOpts(
IO, Options.CheckOptions);
IO.mapOptional("Checks", Options.Checks);
+IO.mapOptional("WarningsAsErrors", Options.WarningsAsErrors);
IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors);
IO.mapOptional("User", Options.User);
@@ -103,6 +104,7 @@
ClangTidyOptions ClangTidyOptions::getDefaults() {
ClangTidyOptions Options;
Options.Checks = "";
+ Options.WarningsAsErrors = "";
Options.HeaderFilterRegex = "";
Options.SystemHeaders = false;
Options.AnalyzeTemporaryDtors = false;
@@ -123,6 +125,10 @@
Result.Checks =
(Result.Checks && !Result.Checks->empty() ?
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs marked 9 inline comments as done. jroelofs added a comment. http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs added a comment. In http://reviews.llvm.org/D15528#312031, @alexfh wrote: > Thank you for explaining. The use case seems to be important enough to > support it. And the solution seems to be good for now. A few concerns: > > 1. `Werrors` isn't a good name for this. The only reason why a similar thing > is called `-Werror` in compilers is that they use `-W` for warnings. I'd > suggest TreatAsErrors, WarningsAsErrors or anything similar (with the proper > casing change for the command line argument). Renamed. > 2. Documentation is missing. Working on that now. http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs added a comment. Thanks for the review! I'll rework this a bit early next week. http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
alexfh added a comment. Thank you for explaining. The use case seems to be important enough to support it. And the solution seems to be good for now. A few concerns: 1. `Werrors` isn't a good name for this. The only reason why a similar thing is called `-Werror` in compilers is that they use `-W` for warnings. I'd suggest TreatAsErrors, WarningsAsErrors or anything similar (with the proper casing change for the command line argument). 2. Documentation is missing. http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs added a comment. In http://reviews.llvm.org/D15528#311019, @alexfh wrote: > Jonathan, can you explain what specific use case does this patch address? Why > one severity level of native clang-tidy warnings (the current situation) is > not enough, and two levels are enough? I have out-of-tree checkers for a very strange out-of-tree target. Some of the checks are on the level of "this should break the build because it cannot possibly work on this target" and others on the level of "tell me about it, but don't force me to fix it". All of these checks are things that don't even remotely apply to other targets. If you're wondering why I haven't hacked up Clang's sema to enforce these constraints, see again: out-of-tree backend... maintaining OOT changes there is expected to be very difficult. Clang-tidy however provides a very nice framework where they can be kept neatly off to the side, away from most of the merge hassles. It'd be nice not to have to run clang-tidy twice & parse its output in order to achieve all of that, hence this patch. Comment at: tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp:7 @@ +6,3 @@ +// CHECK-WARN: warning: namespace 'j' not terminated with a closing comment [llvm-namespace-comment] +// CHECK-WERR: error: namespace 'j' not terminated with a closing comment [llvm-namespace-comment, -Werrors=] + aaron.ballman wrote: > jroelofs wrote: > > aaron.ballman wrote: > > > One of these tests should complete the -Werrors= chunk just to be sure > > > it's getting printed properly. > > Not sure what you mean here. Should I be printing the -Werrors argument as > > the user wrote it? (it isn't currently... this is just a hardcoded string). > Ah, I didn't pick up on that. What is the = for, then? I considered that part of the name of the flag. I can drop that if you think it looks better. Or print out the whole thing as the user wrote it, if you think that's useful (though -checks= and -Werrors= lines are probably pretty long...). http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
alexfh requested changes to this revision. alexfh added a comment. This revision now requires changes to proceed. Jonathan, can you explain what specific use case does this patch address? Why one severity level of native clang-tidy warnings (the current situation) is not enough, and two levels are enough? http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
aaron.ballman added inline comments. Comment at: tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp:7 @@ +6,3 @@ +// CHECK-WARN: warning: namespace 'j' not terminated with a closing comment [llvm-namespace-comment] +// CHECK-WERR: error: namespace 'j' not terminated with a closing comment [llvm-namespace-comment, -Werrors=] + jroelofs wrote: > aaron.ballman wrote: > > jroelofs wrote: > > > aaron.ballman wrote: > > > > One of these tests should complete the -Werrors= chunk just to be sure > > > > it's getting printed properly. > > > Not sure what you mean here. Should I be printing the -Werrors argument > > > as the user wrote it? (it isn't currently... this is just a hardcoded > > > string). > > Ah, I didn't pick up on that. What is the = for, then? > I considered that part of the name of the flag. > > I can drop that if you think it looks better. Or print out the whole thing as > the user wrote it, if you think that's useful (though -checks= and -Werrors= > lines are probably pretty long...). > I would just drop the =; it seems like that suggests there should be more text there. That would also be consistent with the way clang reports -Werror diagnostics (error: unused variable 'i' [-Werror,-Wunused-variable]) http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
alexfh added a comment. In http://reviews.llvm.org/D15528#311053, @jroelofs wrote: > In http://reviews.llvm.org/D15528#311019, @alexfh wrote: > > > Jonathan, can you explain what specific use case does this patch address? > > Why one severity level of native clang-tidy warnings (the current > > situation) is not enough, and two levels are enough? > > > I have out-of-tree checkers for a very strange out-of-tree target. Some of > the checks are on the level of "this should break the build because it cannot > possibly work on this target" and others on the level of "tell me about it, > but don't force me to fix it". All of these checks are things that don't even > remotely apply to other targets. Thank you for the explanation. One more question: do you need to define Werrors differently in different directories? > If you're wondering why I haven't hacked up Clang's sema to enforce these > constraints, see again: out-of-tree backend... maintaining OOT changes there > is expected to be very difficult. No, a sane person wouldn't suggest maintaining a local patch for Clang as a good solution ;) > Clang-tidy however provides a very nice framework where they can be kept > neatly off to the side, away from most of the merge hassles. It's one of the goals of clang-tidy to provide an easy way to maintain out-of-tree checks. > It'd be nice not to have to run clang-tidy twice & parse its output in order > to achieve all of that, hence this patch. Agreed, I want to ensure though, that this is the right approach. In particular, I wonder whether a way to assign labels or severities to clang-tidy diagnostics would be better. Another question is whether we can reuse something from the Clang diagnostic subsystem to map warning levels. http://reviews.llvm.org/D15528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D15528: Teach clang-tidy how to -Werror checks.
jroelofs added a comment.
In http://reviews.llvm.org/D15528#311135, @alexfh wrote:
> In http://reviews.llvm.org/D15528#311053, @jroelofs wrote:
>
> > In http://reviews.llvm.org/D15528#311019, @alexfh wrote:
> >
> > > Jonathan, can you explain what specific use case does this patch address?
> > > Why one severity level of native clang-tidy warnings (the current
> > > situation) is not enough, and two levels are enough?
> >
> >
> > I have out-of-tree checkers for a very strange out-of-tree target. Some of
> > the checks are on the level of "this should break the build because it
> > cannot possibly work on this target" and others on the level of "tell me
> > about it, but don't force me to fix it". All of these checks are things
> > that don't even remotely apply to other targets.
>
>
> Thank you for the explanation. One more question: do you need to define
> Werrors differently in different directories?
Yeah, for things like "if you do this, you will get terribad performance", it's
useful to be able to let the user's build system control them independently
with that kind of granularity. In some places it's acceptable to fail these
checks (i.e. while porting code to this target), and in others it's
mission-critical.
I could see this being useful in other contexts too. For example,
hypothetically, one could imagine turning on the llvm-include-order check for
all of llvm, and then making it -Werror for subdirectories where it has been
cleaned up, ensuring monotonic progress.
>
>
> > If you're wondering why I haven't hacked up Clang's sema to enforce these
> > constraints, see again: out-of-tree backend... maintaining OOT changes
> > there is expected to be very difficult.
>
>
> No, a sane person wouldn't suggest maintaining a local patch for Clang as a
> good solution ;)
:)
>
>
> > Clang-tidy however provides a very nice framework where they can be kept
> > neatly off to the side, away from most of the merge hassles.
>
>
> It's one of the goals of clang-tidy to provide an easy way to maintain
> out-of-tree checks.
Seems to be working out very well on my end... thanks for designing it that way!
>
>
> > It'd be nice not to have to run clang-tidy twice & parse its output in
> > order to achieve all of that, hence this patch.
>
>
> Agreed, I want to ensure though, that this is the right approach. In
> particular, I wonder whether a way to assign labels or severities to
> clang-tidy diagnostics would be better. Another question is whether we can
> reuse something from the Clang diagnostic subsystem to map warning levels.
For my purposes, this kind of trimodal { "it's definitely broken, and cannot
possibly work", "hey, have you considered this?", "the tool didn't spot any
problems" } granularity seems like the right fit, and lines up with Clang's
diagnostics: notes usually stay notes, remarks stay remarks, warnings are
sometimes errors and other times just warnings, and errors are errors.
Something else to think about: grouping a la -Wpedantic/-Wall/-Weverything
might be useful. Some of that can be achieved by giving structure to the names
of the checks, i.e.:
- footarget-perf-check1
- footarget-perf-check2
- footarget-correctness-check1
- footarget-correctness-check2
- footarget-portability-check1
(pardon my vagueness here)
But that kind of breaks down when you start to talk about putting existing
upstream checks into these groups, because then you have to write things like:
`-checks='-*,footarget-portability-*,google-runtime-int-std' (off the cuff
example). Not sure if that's what you meant by "labels", but I see that as
being orthogonal to the question of what a user should be forced to take action
on.
http://reviews.llvm.org/D15528
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
