[cfe-commits] [Patch 2 of 7] -verify fixes and enhancement

[Jordan Rose]

On Jul 3, 2012, at 4:22 PM, Jordan Rose wrote:

> 
> On Jul 3, 2012, at 3:23 PM, Andy Gibbs wrote:
> 
>>>> I see we do check if the location is invalid; I think it's okay to make
>>>> that an assertion. These checks should never come from or refer to lines on
>>>> the command line.
>> 
>> I'm afraid it isn't possible to do an assertion since it is very possible for
>> a test-case writer to provide an invalid line number, for example:
>> 
>> // expected-error@ {{...}}
>> // expected-error at 0 {{...}}
>> // expected-error at -3 {{...}}
>> 
>> (the third will be invalid if the line number of the directive is 3 or less).
> 
> In that case, I'd still prefer to treat these as errors, with a possible exception of "@0" meaning "(frontend)". "@" and "@-3" (or "@5000" or "@+50000") seem like things we can catch up front.

Oops, I see that's already been taken care of. Wait, so why do we need the additional check at the end? Referring to this part of the patch:

> @@ -352,14 +384,18 @@
>  
>    SmallString<256> Fmt;
>    llvm::raw_svector_ostream OS(Fmt);
>    for (DirectiveList::iterator I = DL.begin(), E = DL.end(); I != E; ++I) {
>      Directive& D = **I;
> -    if (D.Location.isInvalid() || !SourceMgr)
> +    if (D.DiagnosticLoc.isInvalid() || !SourceMgr)
>        OS << "\n  (frontend)";
>      else
> -      OS << "\n  Line " << SourceMgr->getPresumedLineNumber(D.Location);
> +      OS << "\n  Line " << SourceMgr->getPresumedLineNumber(D.DiagnosticLoc);
> +    if (!D.DirectiveLoc.isInvalid() && D.DirectiveLoc != D.DiagnosticLoc)
> +      OS << " (directive at "
> +         << SourceMgr->getFilename(D.DirectiveLoc) << ":"
> +         << SourceMgr->getPresumedLineNumber(D.DirectiveLoc) << ")";
>      OS << ": " << D.Text;
>    }
>  
>    Diags.Report(diag::err_verify_inconsistent_diags)

I see that it's not /your/ isInvalid() originally, but I don't think anything should break if you take it away. ...probably.