[cfe-commits] r160068 - in /cfe/trunk: include/clang/Frontend/ lib/Frontend/ test/Frontend/ test/Lexer/ test/Modules/ test/Modules/Inputs/Module.framework/Headers/ test/PCH/ test/Preprocessor/ test/SemaCXX/

[Jordan Rose]

On Jul 11, 2012, at 2:56 PM, Andy Gibbs wrote:

> Hi,
> 
> Firstly, thank you ever so much for all your work getting all these patches
> through.
> 
> I had a quick scan through to see what you changed, and it all seemed fine to
> me, although you could simplify the \r \n algorithm somewhat but this is far
> from critical.  (I'm afraid I come from an embedded background and my mind is
> unfortunately tuned to spotting reducible branch operations.  It is a truly
> terrible habit! :o)

Yeah, the real code to do this is Lexer::SkipEscapedNewLines, which handles trigraphs, but then the fast path gets a little slower. I wanted to handle the really rare case of \r (not \r\n), but maybe that's silly. (The bounds-check is probably a good "don't crash" sort of thing, though.)


> The one change I am in two minds about is this:
> 
>> +  if (!C2.empty())
>> +    if (ParseDirective(C2, ED, SM, CommentBegin, PP.getDiagnostics()))
>> +      if (const FileEntry *E = SM.getFileEntryForID(SM.getFileID(CommentBegin)))
>> +        FilesWithDirectives.insert(E);
> 
> (This appears in a similar way in an earlier location).
> 
> Superficially this change makes sense, and in the end once we've eliminated
> the need for the FilesWithDiagnostics and FilesWithDirectives lists then it
> is irrelevant.  In the meantime, it means that files with directives only
> inside #if blocks will now always get reparsed at the end of processing and
> all the directives contained will be exposed.
> 
> In the case in my patch, if that file at least had a comment outside a skipped
> #if block, then this file was considered to have been parsed for directives
> and was not parsed again.  Its tenuous and I'm wrestling in my head as to
> whether the change in behaviour really matters or not...

I was trying to think about the common case here, where we only have /one/ main file, but then it doesn't matter. (Although too many tests run with -verify just to check that there are no errors...that's probably worth a later cleanup.) 

In the less common case, my code tries to avoid having to grow the SmallPtrSet. We have very few test headers with -verify directives in them (11, according to grep on **/*.h) but quite a few with any comments (80, out of 273 total). Your code tries to avoid reparses, which are definitely more expensive but are quite rare. In fact, none of the 11 headers I found actually have directives within conditionals.

I then searched for files which include non-headers (i.e. files without .h) using this command:

% grep -le '#include ".*\.c"' test/**/*

and similar for .cpp, .m, and .mm, and didn't find any dangerous cases.

Well, in any case, the presence of comments /without/ any directives seems like a very weird thing to switch this behavior on. If this were a real PPCallbacks instance, it would know when #includes are processed, but it isn't.


> Obviously the sooner I can pin down all the exceptions that currently make
> this post-processing necessary, the better.

Here are the tests that fail for me:
    Clang :: ARCMT/GC-check.m
    Clang :: ARCMT/atautorelease-check.m
    Clang :: ARCMT/check-api.m
    Clang :: ARCMT/checking.m
    Clang :: ARCMT/cxx-checking.mm
    Clang :: ARCMT/no-canceling-bridge-to-bridge-cast.m
    Clang :: ARCMT/nonobjc-to-objc-cast-2.m
    Clang :: Modules/lookup.cpp
    Clang :: Modules/objc-categories.m

The ARCMT checks are because arcmt-test has a filtering diagnostic consumer which replays the diagnostics later...not sure what the best way to solve that is. (Perhaps it could /not/ filter for -verify, and the tests would just pass -w?) The Modules problems seem equivalent to the PCH problems and should probably have their checks moved into the main source files.


> Anyway, I am very grateful to you for all your patience while I got the
> patches up to scratch.

And thank you again for writing them in the first place!
Jordan