[cfe-dev] Preprocessor::LookNext

[Chris Lattner]

On Jul 9, 2008, at 10:17 AM, Argiris Kirtzidis wrote:

> Chris Lattner wrote:
>>
>> I like this approach about 10 times better than the other one :) it  
>> is very clever and simple!  The good thing about this is that it is  
>> an easy thing to measure.  Can you whip up a patch that implements  
>> this approach?  I don't think it will have a measurable performance  
>> impact on the preprocessor (unlike the other approach).  If so, I  
>> would be very happy to commit it and simplify the (existing and  
>> future) parser to use it.
>
> The attached "pp-looknext-2.patch" implements  
> Preprocessor::LookNext(), and "parser-nexttoken.patch" modifies the  
> parser to use it.
>
> Let me know if they look ok, with no performance impact.

The difference is *just barely* measurable in my -Eonly worst case.   
It is close enough and good enough to go in, thanks!

Some comments about the patch (pp first):

+
+  /// PeekedToken - Cache the token that was retrieved through  
LookNext().
+  Token PeekedToken;

Please mention in the comment that this is usually not valid, when  
invalid the location is set to invalid.


+  /// LookNext - Returns the next token that would be returned by  
Lex() without
+  /// consuming it.
+  Token LookNext() {

This can return the token by const&.

+    if (PeekedToken.getLocation().isInvalid()) {
+      // We don't have a peeked token that hasn't been consumed yet.
+      // Peek it now.
+      Lex(PeekedToken);
+      // Cache the current Lexer, TokenLexer and set them both to null.
+      // When Lex() is called, PeekedToken will be "consumed".
+      IncludeMacroStack.push_back(IncludeStackInfo(CurLexer,  
CurDirLookup,
+                                                   CurTokenLexer));
+      CurLexer = 0;
+      CurTokenLexer = 0;
+    }

Please move the body of this 'if' out of line.


+  /// ConsumedPeekedToken - Called when Lex() is about to return the  
PeekedToken
+  /// and have it "consumed".
+  void ConsumedPeekedToken() {
+    assert(PeekedToken.getLocation().isValid() && "Confused Peeking?");
+    // Restore CurLexer, TokenLexer.
+    RemoveTopOfLexerStack();
+    PeekedToken.startToken();
+  }

Please move this out of line also.


+++ lib/Lex/PPLexerChange.cpp	(working copy)
@@ -85,6 +85,10 @@
    // size we new to a multiple of 16 tokens.  If the previous buffer  
has space
    // left, we can just grow it.  This means we only have to do the  
new 1/16th as
    // often.
+
+  // Optimized LookAhead(0) case.
+  if (N == 0)
+    return LookNext();

    Token *LookaheadTokens = new Token[N+1];

Is this correct for the LookAhead(1) case?



Parser change:


+  /// NextToken - This peeks ahead one token and returns it without
+  /// consuming it.
+  Token NextToken() {

Should also return via const&.


Otherwise, the parser change looks great, a major simplification!

-Chris