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

[Andy Gibbs]

On Thursday, July 12, 2012 3:48 AM, Jordan Rose wrote:
> 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.)

Yes, no harm there.  As I said, coming from the embedded world I tend to get
stuck in the mindset of the implicit bounds-checking.  Having it explicitly 
is
always better where clock cycles are not so critical.

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

Yes I agree it is a bit weird, but as you say it was to avoid the expensive 
(and
dangerous!) reparse.  Fortunately, all of this list business will disappear 
in
the final revision to come.

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

Well, for the ARCMT checks, test the Part 6 patch -- that should solve 
these.
The solution may not be to everyone's liking, so don't commit it yet.  I can
work on a better patch over the next day or two (depending on the imminent
arrival of a baby).

For the modules tests, I had something in the works a week ago, but put it
aside to get the first round of patches through.  There is good hope
then that it can all be finalised very soon...

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

No problems!  Although it was only the groundwork for some other patches I
have in the pipeline...

Andy