[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

[Vassil Vassilev]

Hi,
On 8/11/12 9:52 PM, Andy Gibbs wrote:
> 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!
I was advocating that IMO it was useful even in Release mode :).
>
> 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.
Fantastic!
>
> Can you give me a few days?
Yes, sure! I can ignore for couple of days the mails from our continuous 
integration :) I hope the rest of the team won't be annoyed very much.
>
> Thanks
> Andy
Cheers,
Vassil