[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,
   Thanks for the quick response!
On 8/10/12 8:22 PM, Andy Gibbs wrote:
> On Friday, August 10, 2012 3:08 PM, Vassil Vassilev wrote:
>> Hi,
>>   If I understand correctly:
>>
>> +      if (E && (FilesParsedForDirectives.count(E)
>> +                  || HS.findModuleForHeader(E)))
>> +        continue;
>>
>>   This doesn't handle the case where the files come from memory
>> buffers  and have only FileIDs. It breaks cling's use case.
>
> 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.
>
>>   Is there any reason for caching FileEntry * rather than FileIDs
>> (typedef llvm::SmallPtrSet<const FileEntry *, 4> 
>> FilesWithDirectivesSet;)
>
> Yes, unfortunately so.  It is not possible to simply cache FileIDs since
> they are specific to a SourceManager instance* and 
> FilesParsedForDirectives
> can span different SourceManagers and therefore must hold the FileEntry
> pointer.  In particular, PCH test-cases will start to fail if we 
> switch to
> caching FileIDs.
>
> [*] I believe/hope I am correct in my assessment here.
I am not sure maybe the experts can tell us ? :)

+    if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(Loc)))
+      FilesList.insert(E);
+  }

Isn't that part bound to the SourceManager instance?
>
> On Friday, August 10, 2012 5:46 PM, Jordan Rose wrote:
>> I think originally it was so that we don't reparse files that have been
>> opened twice (like a file that #includes itself). I'm not sure it's 
>> still
>> necessary, though; it seems like
>> (!E || FilesParsedForDirectives.count(E) || HS.findModuleForHeader(E))
>> would work just as well. Andy?
>
> To make this change would mean memory buffers may not get parsed at 
> all, so
> I personally don't think this is the best approach.  Better is that 
> somehow
> memory buffers are properly marked as parsed when they are parsed the 
> first
> time.  As mentioned before, in the general case I can't do this by FileID
> since these are dependent on the SourceManager instance.  However, it was
> anticipating this sort of problem that I added the "appendParsedFile"
> function into VerifyDiagnosticConsumer, so that external code could 
> mark a
> file as parsed even though it wasn't picked up though PPCallbacks 
> (which is
> the standard mechanism).  My original vision for this was for handling
> module files, but here is another potential use.  However, at the moment,
> "appendParsedFile" requires a FileEntry pointer.
>
> Vassil, if I were to make an "appendParsedFile" that took FileIDs 
> (assuming
> you don't change SourceManager), would you be able to reach the 
> instance of
> VerifyDiagnosticConsumer in order to use it?  (As a side point, I 
> worked on
> a patch to allow isa/dyn_cast/etc. between DiagnosticConsumers, 
> although it
> didn't have a useful enough purpose to be committed before, but I 
> mention it
> here in case it is of use to you.)
My preliminary opinion is if appendParsedFile takes a FileID I don't 
think it would help very much. I am not sure what the method 
implementation would be (that's why I say my preliminary opinion :)).
I don't think isa/dyn_cast is the right way to tackle the issue and to 
go in general. The verifier shouldn't be treated in different way than 
any other diagnostic consumer.
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?
>
> If you can give me some short sample code to play with on how you are 
> using
> memory buffers with -verify, then I will put my thoughts to it over the
> weekend while rocking a baby to sleep...  I already have some ideas but I
> want to think further before sticking my neck out too far!  :o)
As I said above I can't. Maybe the best is download cling next to clang 
and run the tests.
>
> 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?
>
> Cheers
> Andy
>
Thanks,
Vassil