[PATCH] D151320: [clang] Add `// expected-maybe-no-diagnostics` comment to VerifyDiagnosticConsumer

[Joel E. Denny via Phabricator via cfe-commits]

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