[PATCH] [PATCH] [CONCEPTS] Parsing of concept keyword

[Richard Smith]

================
Comment at: test/Parser/cxx-concepts-value-function.cpp:11
@@ +10,3 @@
+
+#ifdef DIAG
+
----------------
hubert.reinterpretcast wrote:
> rsmith wrote:
> > nwilson wrote:
> > > rsmith wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > nwilson wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > -DDIAG=0 would still trigger the #ifdef DIAG here (which explains why the test file might pass with expected-no-diagnostics).
> > > > > > > I meant //without// `expected-no-diagnostics` somewhere.
> > > > > > Hmm, yeah, I missed that and thought it was working. Is there a better way to use/suppress the diagnostic cases?
> > > > > `#if DIAG` instead of `#ifdef` would work. I guess `expected-no-diagnostics` could be in the `#else` for that.
> > > > Just run the test once, and remove the `#ifdef`.
> > > I'll submit a patch for this shortly so you guys can see if this explanation doesn't make sense. But, how about if I use the directives likes this:
> > > 
> > > #if DIAG == 0
> > > // RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s -DDIAG=0
> > > // expected-no-diagnostics
> > > 
> > > *** test cases ***
> > > 
> > > #elif DIAG == 1
> > > // RUN: %clang_cc1 -std=c++14 -fconcepts-ts -x c++ -verify %s -DDIAG=1
> > > 
> > > *** test cases ***
> > > 
> > > #endif
> > > 
> > > 
> > What are you trying to achieve by running the test twice?
> The presence of errors in a file may prevent further processing. Such further processing is not guaranteed to apply only to code which occurs lexically after the location of the error. By avoiding diagnostic cases in one of the runs, we can be more confident that the cases which should not receive diagnostics would not do so (instead of merely having the unexpected diagnostics conveniently preempted). 
Failure to recover properly from an error would be a bug -- the parse of later unrelated code should not be affected by an earlier failure, except in extreme cases. The same argument would also imply that we should not have two "expected to fail" tests in a row in one test file, because the earlier failure could conceivably prevent the later failure from happening (or the later failure might only happen when the earlier failure does).

The valid cases will be tested more thoroughly in higher layers (for instance, testing that semantic analysis can use a concept to select an overload and that IR generation can generate correct IR for use of a concept), so I'm not at all worried about those later tests failing to catch a bug where a trivial concept definition leads to an unexpected error.

As a more general observation: there is an established pattern for these kinds of test (and many other things) in Clang. If our established pattern is wrong, we should deal with that by discussing the pattern and how to fix it, then fixing it globally, not by deviating from that pattern in an individual change (that will lead to a fragmented and harder-to-maintain codebase).

http://reviews.llvm.org/D10528

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/