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.
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. 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.) 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) 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. Cheers Andy
verify-fileid.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
