[cfe-commits] r161650 - in /cfe/trunk: include/clang/Frontend/VerifyDiagnosticConsumer.h lib/ARCMigrate/ARCMT.cpp lib/Frontend/VerifyDiagnosticConsumer.cpp test/ARCMT/verify.m test/ASTMerge/function.c test/Frontend/verify.c test/Frontend/verify2.c test/Frontend/veri

[Jordan Rose]

Hm. Now I'm having second thoughts about this:

> fatal_error:
>       llvm::report_fatal_error(Twine("presence of -verify directives cannot be "
>                                      "verified during post-processing check of ",
>                                StringRef(FE ? FE->getName() : "(unknown)")));

This happens if we don't have a SourceManager or Preprocessor available. But what if we really /don't/ have a Preprocessor available? Do we really want to complain about this? A buffer that doesn't need to be preprocessed probably won't contain -verify directives. Or the directives could be added in some other way. Do we want to /require/ that a preprocessor be available just to check the diagnostics we have?

Perhaps the compromise here is to do the checking only after we've checked the current set of diagnostics against the current set of directives. This misses the case where someone put an expected-warning in an unparsed file that refers to a diagnostic in a parsed file, but that seems pretty rare.

To put it another way, if all diagnostics are accounted for in a PCH file, maybe I don't care that the PCH file has its own expectations. I can see how that would be arguable, though.

(We'd miss a few intended "expected but not seen", but never miss "seen but not expected".)

Jordan


On Aug 15, 2012, at 22:28 , Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Thursday, August 16, 2012 4:05 AM, Jordan Rose wrote:
>> This looks pretty good to me! I initially thought we could pull all the
>> SourceManager stuff under NDEBUG, but it does make sense to have it used
>> for the actual emitting of errors at the end, rather than pulling it
>> from the preprocessor.
> 
> I did consider keeping the map between FileID and FileEntry* and also
> marking module header files as parsed at the point they are seen so that
> SrcManager also dropped out in the NDEBUG block in CheckDiagnostics, but
> I wasn't convinced the additional complexity and "baggage" was warranted
> so I kept with the simple approach for now.
> 
>> I'll look at this again tomorrow morning (time zone PDT) and commit it
>> then if it all looks good.
> 
> Great, thanks!  :o)
> 
> Plus, it means I can also get in this bit that slipped the net:
> 
> --- tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
> +++ tools/clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
> @@ -37,10 +37,11 @@
> }
> 
> VerifyDiagnosticConsumer::~VerifyDiagnosticConsumer() {
>   assert(!ActiveSourceFiles && "Incomplete parsing of source files!");
>   assert(!CurrentPreprocessor && "CurrentPreprocessor should be invalid!");
> +  SrcManager = 0;
>   CheckDiagnostics();  
>   Diags.takeClient();
>   if (OwnsPrimaryClient)
>     delete PrimaryClient;
> }
> 
> 
> SrcManager needs to be cleared before CheckDiagnostics because it will
> certainly be pointing to some invalid location by this stage.
> 
> Thanks again,
> 
> Andy
> 
>