[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 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

 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: verify-fileid.diff
Type: application/octet-stream
Size: 2903 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120810/be78047a/attachment.obj>