jdenny accepted this revision.
jdenny added inline comments.
================
Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:468
+ Status = VerifyDiagnosticConsumer::HasExpectedMaybeNoDiagnostics;
+ continue;
+ } else if (DToken.endswith(DType="-no-diagnostics")) {
----------------
Endill wrote:
> 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?
> 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,
That line is optimized for the case of a single prefix: it ignores text without
that prefix. If there are multiple prefixes, then it searches for a more
general pattern and waits until the `std::binary_search` call below to worry
about the prefix.
> so I wrote a more comprehensive set of tests. Does this address your concern?
Yes, and thanks for the extra testing.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151320/new/
https://reviews.llvm.org/D151320
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
