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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
