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

[Vlad Serebrennikov via Phabricator via cfe-commits]

Endill 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
----------------
jdenny wrote:
> 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.
> 
Yeah, it's better the way you propose.


================
Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:470-471
       NoDiag = true;
       if (D.RegexKind)
         continue;
     }
----------------
jdenny wrote:
> Shouldn't `expected-maybe-no-diagnostics` have this too so that `expected-maybe-no-diagnostics-re` is skipped?  Please add a test.
You're right, there's no good reason for us not to do the same. Though I don't like that we silently skip over not-too-sensical `// expected-no-diagnostics-re`. I'll prepare a follow-up patch that makes `-re` a hard error for those two special directives.

Tested in A3


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


================
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:
> 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
Sorry, I misunderstood your initial comment. It's fixed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320