<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Feb 5, 2014 at 2:36 PM, Harald van Dijk <span dir="ltr"><<a href="mailto:harald@gigawatt.nl" target="_blank">harald@gigawatt.nl</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On 04/02/14 22:24, Richard Smith wrote:<br>
> On Tue, Feb 4, 2014 at 1:13 PM, Harald van Dijk <<a href="mailto:harald@gigawatt.nl">harald@gigawatt.nl</a><br>
</div><div class="im">> <mailto:<a href="mailto:harald@gigawatt.nl">harald@gigawatt.nl</a>>> wrote:<br>
><br>
> On 04/02/14 20:25, Justin Bogner wrote:<br>
</div>> > Harald van Dijk <<a href="mailto:harald@gigawatt.nl">harald@gigawatt.nl</a> <mailto:<a href="mailto:harald@gigawatt.nl">harald@gigawatt.nl</a>>><br>
<div><div class="h5">> writes:<br>
> >> Attached are my updated patches intended to fix various whitespace<br>
> >> issues in clang, modified to take Justin Bogner's comments into<br>
> account.<br>
> >> They are intended to ensure that whitespace is not inappropriately<br>
> >> removed just because a macro or macro argument expansion is<br>
> empty, and<br>
> >> does get removed during token pasting.<br>
> ><br>
> > I've committed the first four patches for you: r200785 through<br>
> r200788.<br>
><br>
> Thanks!<br>
><br>
> >> I have moved the handling of invalid token pastes from<br>
> >> TokenLexer::ExpandFunctionArguments to TokenLexer::Lex, so that<br>
> it works<br>
> >> for both object- and function-like macros, and both when ##'s<br>
> operands<br>
> >> use macro parameters and when they don't.<br>
> ><br>
> > Given that the tests needed to be changed and the behaviour clearly<br>
> > hasn't followed the comment in a while, I'm not entirely convinced<br>
> this<br>
> > is the right thing to do. Could the comment simply be wrong? Are<br>
> people<br>
> > relying on one behaviour or the other for invalid token pastes in<br>
> > assembler-with-cpp?<br>
> ><br>
> > Basically, is there a way to objectively say one of these<br>
> behaviours is<br>
> > better than the other?<br>
><br>
> Having looked closer, the current behaviour is inconsistent in a way<br>
> that cannot really be explained to someone not familiar with a few<br>
> implementation details. Given<br>
><br>
> #define foo(x) (. ## x . ## y)<br>
><br>
> foo(y) actually expands to (.y . y) in assembler-with-cpp mode, with or<br>
> without my first four patches. That first result is what the comment<br>
> refers to; could the fact that the second result is different be an<br>
> oversight? There does not seem to be a reason for this difference, at<br>
> least. Surprisingly though, this is exactly what GCC does too.<br>
><br>
> My last patch would have turned this into (.y .y), removing the second<br>
> space. That makes sense to me. (. y . y) could also be a perfectly<br>
> sensible result.<br>
><br>
><br>
> I think (.y .y) is probably the best answer here (consistently remove<br>
> whitespace on both sides of ## wherever it appears) -- this also<br>
> probably better matches what MSVC does (where token-paste can result in<br>
> multiple tokens, and seems to act as if the tokens were merely abutted<br>
> with no adjacent whitespace). This would also behave more consistently<br>
> when processing languages with a different lexical structure from C --<br>
> in a language where an identifier can start with a period, (.y .y) seems<br>
> to unambiguously be the right answer.<br>
<br>
</div></div>That is a good point about MSVC. There is actually one test case<br>
affected by this change, using -fms-extensions, where the behaviour did<br>
not match that of MSVC, and does now, at least for the compiler that<br>
comes with Visual Studio 2013.<br>
<div class="im"><br>
> I suspect, but do not actually know, that nobody really uses . ## foo<br>
> unless foo is a macro argument, because when foo is not a macro<br>
> argument, there is no point in using ## in the first place. It would<br>
> explain why this went unnoticed for so long. And I can imagine a few<br>
> cases where this would be useful: assembler directives that have subtly<br>
> different syntax, depending on the assembler used. x y would be<br>
> insufficient if x is ., if y is size, and if .size is a directive, but .<br>
> size is a syntax error. ## would work here.<br>
><br>
> Even if clang's behaviour should change, though, my patch does not have<br>
> adequate testing for these special cases, so shouldn't be applied as is.<br>
> Should I work on better tests, or do you think it is more likely that<br>
> the behaviour should remain unchanged and get decent testing?<br>
><br>
><br>
> There's a risk of breaking someone's assumption by making a change here<br>
> (especially since we'd be introducing a difference from GCC) but I think<br>
> the new behavior is much more defensible, and there's probably no way to<br>
> find out without trying it.<br>
<br>
</div>All right. I have now taken a closer look at the test cases where the<br>
behaviour changes, and noticed that for blargh ## $foo -> blargh $foo it<br>
would not be right to simply change the test case to blargh$foo, as that<br>
misses the point of the test.<br></blockquote><div><br></div><div>Right, I see, it's just making sure that -fdollars-in-identifiers allows the paste to work. I like your fix to the test here.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
There was also already a test for . ## foo, but it only tested the case<br>
where foo is a macro argument. I have extended that test to also check<br>
what happens when foo is not a macro argument.<br></blockquote><div><br></div><div>Can you also add a test for the case where the . comes from a macro argument? May as well be thorough =)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
How does the attached patch look? I've re-tested it on sources of today,<br>
but on top of my patch in the "r200787 - Fix whitespace handling in<br>
empty macro expansions" thread (after your okay for that).</blockquote><div><br></div><div>Checking whether the previous token was a ## worries me a little. What about cases like this:</div><div><br></div><div>#define foo(x, y) x####y</div>
<div>foo(, z)</div><div><br></div><div>This expands to "## z" under our current left-to-right pasting strategy, but I think your patch drops the space. Amusingly, "##z" appears to be the right answer under a right-to-left pasting strategy, so maybe that's OK? Both GCC and EDG expand this to just "z"; the standard is not spectacularly clear here, but I think we're right, because a ## produced by pasting a placemarker token onto a ## is not a token in the replacement list (that token is already gone).</div>
<div><br></div><div>If you want to say that we don't care about this case, I could live with that =)</div></div></div></div>