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