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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits