[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/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+        Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+      continue;
+    } else if (DToken.endswith(DType="-no-diagnostics")) {
----------------
jdenny wrote:
> Endill wrote:
> > jdenny wrote:
> > > Endill wrote:
> > > > 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
> > > Does A2 trigger the original bug?  I think there must be multiple prefixes for this fix to matter.
> > I think prefix checking below works for a single prefix as well: https://github.com/llvm/llvm-project/blob/5217498dc88aa2de2b728462205ffa8b01d96cab/clang/lib/Frontend/CompilerInvocation.cpp#LL2382C47-L2382C47
> I downloaded this version of your patch: <https://reviews.llvm.org/D151320?vs=on&id=525216>.  That version has the bug discussed in this comment thread.
> 
> I applied that patch and added test A2.  A2 passed.  That means A2 does not reproduce the bug it was intended to reproduce.
> 
> Then I changed A2 to have `-verify=foo,bar`.  A2 then failed because it then reproduced the bug.  (And of course with the current version of your patch, it passes because you've fixed the bug.)
> 
> Please add such a test.
I replaced A2 test with more comprehensive E set of tests.

Surprisingly to me, `-verify` and `-verify=foo,bar` work quite differently because of `PH.Search()` call 40 lines above. I can't say I have full understanding what's going on, so I wrote a more comprehensive set of tests. Does this address your concern?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151320