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

[Vlad Serebrennikov via Phabricator via cfe-commits]

Endill added a comment.

Thank you for the review!



================
Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+        Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+      continue;
+    } else if (DToken.endswith(DType="-no-diagnostics")) {
----------------
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!


================
Comment at: clang/test/Frontend/verify-maybe-no-diagnostics.c:38
+#endif
+
+#ifdef TEST_D1
----------------
jdenny wrote:
> Please test this case:
> 
> ```
> #ifdef TEST_C3
> // expected-maybe-no-diagnostics
> #error test_c3
> #endif
> ```
> 
> That is, does Clang actually fail in this case as expected because there's an error and no corresponding `expected-error` directive?  Or does it ignore the error because of the `expected-maybe-no-diagnostics` directive?
Added C2 and C4 tests that should cover that.

> That is, does Clang actually fail in this case as expected because there's an error and no corresponding expected-error directive? Or does it ignore the error because of the expected-maybe-no-diagnostics directive?

It works like there's no `// expected-maybe-no-diagnostics`. That is, failing because there's no matching `expected-error` directive for an error produced.


================
Comment at: clang/test/Frontend/verify-maybe-no-diagnostics.c:51-52
+#ifdef TEST_D2
+// expected-maybe-no-diagnostics
+// expected-no-diagnostics
+#error test_d2
----------------
jdenny wrote:
> So `expected-no-diagnostics` overrides `expected-maybe-no-diagnostics`.  In your use case, omitting one or the other is not always possible?
Yes, it works the way you describe.
In our case, `// expected-maybe-no-diagnostic` comes into hundreds of test cases from force include. It's very much possible for us to ensure that only one of them is present, by banning `expected-no-diagnostics`.

`expected-maybe-no-diagnostic` is by design low-priority and no-friction directive, contrary to strict nature of all other directives. That's why it could be very easily overridden.


================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320