[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

[Jordan Rose]

On Aug 17, 2012, at 8:15 , Andy Gibbs <andyg1001 at hotmail.co.uk> wrote:

> On Thursday, August 16, 2012 8:12 PM, Jordan Rose wrote:
>> On Aug 16, 2012, at 11:00 , Andy Gibbs wrote:
>>> ---When we get a diagnostic---
>>> if (is a macro expansion)
>>> ignore [[big culprit this one!]]
>> 
>> Why not look at where the macro is instantiated?
> 
> Is it necessary?  A macro expansion itself will not contain comments,
> and hence no directive?... (that's right isn't it?)

I'll agree that directives should not come from macros, but a macro /instantiation/ can result in diagnostics (that's this case), in which case we definitely want to make sure that the file containing the macro instantiation location (not its definition location!) has been parsed for directives. I'm not sure we'll ever actually see an issue like this, but it seems better than dropping it on the floor.

I just realized the DiagnosticsEngine has a SourceManager. Does it make sense for you to use that instead of keeping track of it separately?


Nits:

+  class UnparsedFileStatus
+          : private llvm::PointerIntPair<const FileEntry*, 1, bool> {
+    typedef llvm::PointerIntPair<const FileEntry*, 1, bool> base;

Probably should have commented on this earlier, but private inheritance is pretty rare in LLVM. We tend to just use a member variable instead.


+    ParsedFiles.insert(std::make_pair(FID, SM.getFileEntryForID(FID)));

Since there's no search or result checking here, I think it's a lot cleaner to use subscripting, i.e. "ParsedFiles[FID] …"


+    // Add FileID to unparsed.
+    UnparsedFiles.insert(std::make_pair(FID,
+                         UnparsedFileStatus(SM.getFileEntryForID(FID),
+                                            FoundDirectives)));

Ditto.