[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 andyg1001 at hotmail.co.uk
Fri Aug 17 13:32:07 PDT 2012


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
 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: verify.diff
Type: application/octet-stream
Size: 14574 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120817/4c0af078/attachment.obj>


More information about the cfe-commits mailing list