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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
