r200787 - Fix whitespace handling in empty macro expansions

[Harald van Dijk]

On 04/02/14 22:48, Justin Bogner wrote:
> Richard Smith <richard at metafoo.co.uk> writes:
>> On Tue, Feb 4, 2014 at 11:18 AM, Justin Bogner <mail at justinbogner.com> wrote:
>>     Fix whitespace handling in empty macro expansions
>>    
>>     When a macro expansion does not result in any tokens, and the macro name
>>     is preceded by whitespace, the whitespace should be passed to the first
>>     token that follows the macro expansion. Similarly when a macro expansion
>>     ends with a placemarker token, and that placemarker token is preceded by
>>     whitespace. This worked already for top-level macro expansions, but is
>>     now extended to also work for nested macro expansions.
>>    
>>     Patch by Harald van Dijk!
>>    
>>     Modified:
>>         cfe/trunk/lib/Lex/TokenLexer.cpp
>>         cfe/trunk/test/Preprocessor/macro_space.c
>>    
>>     Modified: cfe/trunk/lib/Lex/TokenLexer.cpp
>>     URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/TokenLexer.cpp?
>>     rev=200787&r1=200786&r2=200787&view=diff
>>     ==========================================================================
>>     ====
>>     --- cfe/trunk/lib/Lex/TokenLexer.cpp (original)
>>     +++ cfe/trunk/lib/Lex/TokenLexer.cpp Tue Feb  4 13:18:35 2014
>>     @@ -477,9 +477,13 @@ bool TokenLexer::Lex(Token &Tok) {
>>        if (isFirstToken) {
>>          Tok.setFlagValue(Token::StartOfLine , AtStartOfLine);
>>          Tok.setFlagValue(Token::LeadingSpace, HasLeadingSpace);
>>     -    AtStartOfLine = false;
>>     -    HasLeadingSpace = false;
>>     +  } else {
>>     +    // If this is not the first token, we may still need to pass through
>>     +    // leading whitespace if we've expanded a macro.
>>     +    if (HasLeadingSpace) Tok.setFlag(Token::LeadingSpace);
>>
>> Do we need to do this for AtStartOfLine too? What happens if we have a macro
>> at the start of a line that expands to no tokens, within a TokenLexer? (I
>> guess this can't happen within a macro expansion, since no tokens there are
>> ever at the start of a line after line-splicing, but we use TokenLexers in
>> other cases too).
> 
> CCing Harald.
> 
> Handling AtStartOfLine as well does sound pretty reasonable to me, but
> it's difficult to come up with a case where this actually applies.
> Without a concrete example, I don't have a good way to judge this.

It can happen even in a macro expansion:

#define foo() bar() .
#define bar()
foo()

The question is whether the resulting . should be treated as at the
start of the line. I was initially unable to come up with a test case
where it mattered, but am able to come up with one now, because error
handling is different depending on whether certain tokens start on the
next line:

int a, int b; // expected-error {{expected identifier or '('}}
// expected-error at -1 {{expected ';' after top level declarator}}

int c, // expected-error {{expected ';' at end of declaration}}
int d;

#define foo() bar() int f;
#define bar()
int e, // expected-error {{expected ';' at end of declaration}}
foo()

This fails, should pass, and setting Token::StartOfLine is indeed enough
to make it pass, like so:

diff --git a/lib/Lex/TokenLexer.cpp b/lib/Lex/TokenLexer.cpp
index 9913f30..543ca92 100644
--- a/lib/Lex/TokenLexer.cpp
+++ b/lib/Lex/TokenLexer.cpp
@@ -474,6 +474,7 @@ bool TokenLexer::Lex(Token &Tok) {
   } else {
     // If this is not the first token, we may still need to pass through
     // leading whitespace if we've expanded a macro.
+    if (AtStartOfLine) Tok.setFlag(Token::StartOfLine);
     if (HasLeadingSpace) Tok.setFlag(Token::LeadingSpace);
   }
   AtStartOfLine = false;


I am not happy with my test case, though. Is there an easier way to
verify that this works as intended?

Cheers,
Harald van Dijk