[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

[Andy Gibbs]

On Saturday, August 11, 2012 6:03 PM, Vassil Vassilev wrote:
> On 8/10/12 8:22 PM, Andy Gibbs wrote:
>> [...snip...]
>> I need to look into this for you.  Certainly this is a use-case I didn't
>> consider -- my apologies.  Would you be willing to post some sample code
>> and I'll work a proper test-case from it so that I can include this use-
>> case directly into clang's test-suite.
> Unfortunately it is not easy to construct a test case in clang. The 
> closest similar use in clang is when you start it without any file to 
> compile but with "-". However it is by far different from what we have. 
> IMO the best is just download cling and run make test (the same way you do 
> it for clang). Sorry I can't be more helpful here, but I can't come up 
> with anything else.

That's ok.  I've get a copy of cling and do as you say.

> +    if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(Loc)))
> +      FilesList.insert(E);
> +  }
>
> Isn't that part bound to the SourceManager instance?

No, FileEntry instances are global (cached).

> I don't understand why you even need to split the logic into release and 
> debug part (#ifdef NDebug). What is the intent of doing it?

The debug part is an expensive secondary check that all expected-*
directives have been captured.  This was very important in picking up
the various places inside clang where holes previously existed.  In the
release build such a check is not necessary since in the debug build the
check has shown that everything is captured. As such, unless there is a
really good and compelling reason, I would be loathed to remove this
secondary check.  After all, one could (and I would!) argue that the
problem you are having here is exactly why such a check is necessary.
The answer is to fix your problem, not remove the check that makes it
(and maybe other things) visible!

>> In the meantime, attached is a patch (** not to be committed, Jordan! **)
>> that simply changes FilesParsedForDirectives from FileEntry* to FileID.
>> Like I said, PCH test-cases now fail, so its not a final solution, but 
>> I'd be interested to know if you can use "appendParsedFile" to get round
>> your problem.
> Yes this patch works perfectly for our use case. As a matter of fact I did 
> the same, but I saw couple of failures in clang (as you said: modules and 
> PCHs) and I didn't have time to think how to fix it. That is why I want to 
> discuss it first. Can't we add an extra check whether the FileID has 
> FileEntry and that FileEntry is PCH or something like that?

I have a solution in mind which was forming when I posted this patch, but
I also want to check it thoroughly and generate a test-case which can be
part of the clang test-suite ongoing which encapsulates your usage.

Can you give me a few days?

Thanks
Andy