On Friday, August 17, 2012 7:01 PM, Jordan Rose wrote:
On Aug 17, 2012, at 8:15 , Andy Gibbs 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.
No, I take your point. I've added in a simple "get to top caller" loop to pull this location out.
I just realized the DiagnosticsEngine has a SourceManager. Does it make sense for you to use that instead of keeping track of it separately?
You are right: I'll use this, if it is available, as an initial value. I still want to track since I want to ensure it cannot be changed once set (it would invalidate the link forged between FileID and FileEntry!).
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.
Done!
+ 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] …"
Actually, I haven't done this since using "insert" has the advantage that it only inserts and never updates. If you think this is not a good enough reason, I can change this.
+ // Add FileID to unparsed. + UnparsedFiles.insert(std::make_pair(FID, + UnparsedFileStatus(SM.getFileEntryForID(FID), + FoundDirectives))); Ditto.
And in this case, it requires UnparsedFileStatus to have a default ctor and I'd avoided this so far. This again is not something I'll make a stand over, so if you think subscripting is still the better way, I can change this too. Anyway, I'm still holding my breath for the second reading :o) The latest patch is attached. Cheers Andy
verify.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
