jdenny added inline comments.
================
Comment at: clang/include/clang/Frontend/VerifyDiagnosticConsumer.h:186-202
+/// Additionally, you can use:
+///
+/// \code
+/// // expected-maybe-no-diagnostics
+/// \endcode
+///
+/// to specify that a file with no "expected-*" comments should pass when no
----------------
Thanks for adding documentation.
I feel that this edit makes the behavior a little clearer, and it clarifies
what happens when "expected-no-diagnostics" and "expected-maybe-no-diagnostics"
are combined.
Also, the original text had:
> but they do not fail automatically due to a combination of
> "expected-no-diagnostics" and "expected-*" within the same test
That reads to me like it's ok to combine "expected-no-diagnostics" and
"expected-*" directives specifying diagnostics. Hopefully this edit clarifies
that point.
================
Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:470-471
NoDiag = true;
if (D.RegexKind)
continue;
}
----------------
Shouldn't `expected-maybe-no-diagnostics` have this too so that
`expected-maybe-no-diagnostics-re` is skipped? Please add a test.
================
Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+ Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+ continue;
+ } else if (DToken.endswith(DType="-no-diagnostics")) {
----------------
Endill wrote:
> jdenny wrote:
> > This `continue` skips the prefix checking below, which is important when
> > there are multiple prefixes active (e.g., `-verify=foo,bar`). That is, any
> > old `BOGUS-maybe-no-diagnostics` will be effective then.
> This should be fixed now. Thank you for spotting this!
Thanks for the fix. Please add a test so this bug doesn't pop up again.
================
Comment at: clang/test/Frontend/verify-maybe-no-diagnostics.c:104
+// D6-CHECK: error: 'error' diagnostics seen but not expected:
+// D6-CHECK-NEXT: {{.*}} 'expected-no-diagnostics' directive cannot follow
other expected directives
+// D6-CHECK-NEXT: 1 error generated.
----------------
Endill wrote:
> jdenny wrote:
> > This diagnostic is confusing. Should we add "except
> > 'expected-maybe-no-diagnostics'"?
> As I mentioned in another comment, `maybe-no-diagnostics` has the lowest
> priority, and doesn't have strict and declarative nature, unlike any other
> directive. That's why it should never be expected (and ideally very rarely
> used).
>
> The purpose of all the tests I added is to ensure `expected-no-diagnostic`
> doesn't affect existing directives and their interaction in any way.
I don't see how that addresses my concern. Maybe it's because, after the
latest edits, phab shifted my comment to the wrong test. I was originally
commenting on this:
> 'expected-no-diagnostics' directive cannot follow other expected directives
This message is now incorrect. `expected-no-diagnostics` //can// follow
`expected-maybe-no-diagnostics`. What if we reword as follows?
> 'expected-no-diagnostics' directive cannot follow directives that expect
> diagnostics
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151320/new/
https://reviews.llvm.org/D151320
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
