<div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">I don't have any say in the AST code.  That would have to involve one of the owners (and I don't know who those are) and it's not a simple matter, because so much of the compiler is connected to the AST. If you're thinking of such a change I would start a new thread on this list explicitly raising the issue.</div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div><div class="gmail_default" style="font-family:arial,helvetica,sans-serif">Regarding that change -- I'm not sure how relevant it is, given that it's from back in 2006.  But, it is interesting to see and probably indicates there's a good reason for the discrepancy (or, at least, there was at some point).</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 11, 2020 at 5:35 PM <<a href="mailto:alex@lanin.de">alex@lanin.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div lang="DE"><div class="gmail-m_-8805088585333942563WordSection1"><p class="MsoNormal"><span lang="EN-US">Ok got it!</span><span lang="EN-US"><u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US">I thought all Expressions look like that missing ExprStmt.<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US">There are Statements and Expressions and I initially assumed Expressions look like Statements, but that’s just one sub/supertype of Expr.<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US">Looking into the DoWhile topic I found a corresponding commit:<u></u><u></u></span></p><p class="MsoNormal"><a href="https://github.com/llvm/llvm-project/commit/503fadc90f397a773ac17f40f72ca3e81cba2c49" target="_blank"><span lang="EN-US">https://github.com/llvm/llvm-project/commit/503fadc90f397a773ac17f40f72ca3e81cba2c49</span></a><span lang="EN-US"><u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US">Interestingly the statements touched in that commit have some high overlap with my “strange behavior” list in </span><a href="https://reviews.llvm.org/D75813#change-Bc7Hs8CXH7Z9" target="_blank"><span lang="EN-US">https://reviews.llvm.org/D75813#change-Bc7Hs8CXH7Z9</span></a><span lang="EN-US"><u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US">I came up with my list by comparing results of an AST based approach and a token based approach - over llvm codebase.<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US">Apparently there are no gotos in the part which I have looked at ;-)<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US">I think I could change ParseStmt.cpp for DoWhile, GotoStmt, ContinueStmt, BreakStmt, ReturnStmt, AsmStmt and SEHLeaveStmt.<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US">Looks rather trivial (so it probably isn’t).<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US">But I would have to change Stmt.h as well and add an SemiLoc or something to all of them.<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US">For example BreakStmt doesn’t have any secondary SourceLocation, others already store RParenLoc – but that doesn’t help.<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US">Does that make sense? I would still need to keep my getUnifiedEndLoc, but it could be simplified to getStmtLikeEndLocForStmtLikeExpr.<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US">Alex<u></u><u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><span lang="EN-US"><u></u> <u></u></span></p><p class="MsoNormal"><b>Von:</b> Yitzhak Mandelbaum <<a href="mailto:yitzhakm@google.com" target="_blank">yitzhakm@google.com</a>> <br><b>Gesendet:</b> Mittwoch, 11. März 2020 18:47<br><b>An:</b> Alexander Lanin <<a href="mailto:alex@lanin.de" target="_blank">alex@lanin.de</a>><br><b>Cc:</b> Clang Dev <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>><br><b>Betreff:</b> Re: [cfe-dev] Stmt.getEndLoc() vs semicolon<u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><div><div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif">responses inline...<u></u><u></u></span></p></div></div><p class="MsoNormal"><u></u> <u></u></p><div><div><p class="MsoNormal">On Sat, Feb 29, 2020 at 3:42 PM <<a href="mailto:alex@lanin.de" target="_blank">alex@lanin.de</a>> wrote:<u></u><u></u></p></div><blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm"><div><div><p><span lang="EN-GB">Hi,</span><u></u><u></u></p><p><span lang="EN-GB"> </span><u></u><u></u></p><p><span lang="EN-GB">>> When an expression is used in a context expecting a statement and is trailed by a semicolon, the range will not include the semicolon</span><u></u><u></u></p><p><span lang="EN-GB">Assuming there is a reason for doing that, this explains the effects. It certainly sounds easier to extend Expr range instead of wrapping it in ExprStmt...</span><u></u><u></u></p></div></div></blockquote><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif">The problem is context. Consider expression `f(e)` in two cases:<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif"><u></u> <u></u></span></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif">1. `f(e);`<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif">2. `return f(e) + 3;`<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif"><u></u> <u></u></span></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif">What is the range of `f(e)`? Is it wider in the first case than the second? if so, will this surprise/break code that might assume it invariant?  But, even worse, in case 1, how do you know which range the user wants? If I'm replacing statements, then I'll want through the semicolon, but if I'm updating call expressions, I'll only want the call itself.<u></u><u></u></span></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif"><u></u> <u></u></span></p></div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif">So, both context and intent matter. An ExprStmt would clearly separate the two.<u></u><u></u></span></p></div><blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm"><div><div><p><span lang="EN-GB"> </span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB">>> Expr vs Stmt</span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB">Turns out DoStmt is also fun as the conditions is an Expr.</span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB" style="color:blue">do</span><span lang="EN-GB" style="color:black"> {} </span><span lang="EN-GB" style="color:blue">while</span><span lang="EN-GB" style="color:black">(</span><span lang="EN-GB" style="color:blue">false</span><span lang="EN-GB" style="color:black">)</span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB" style="color:green">/* Comment is not part of DoStmt,</span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB" style="color:green">although it's before the semicolon*/</span><span lang="EN-GB" style="color:black">;</span><u></u><u></u></p></div></div></blockquote><div><p class="MsoNormal"><span class="gmail-m_-8805088585333942563gmaildefault"><span style="font-family:Arial,sans-serif">Good catch!</span></span> <span class="gmail-m_-8805088585333942563gmaildefault"><span style="font-family:Arial,sans-serif">But, I don't think that's the reason, because the condition is clearly delineated by the parentheses. It just seems to be an inconsistency. I can't see any reason that the comment and semi are excluded here while included in the DeclStmt. I think this should be changed.</span></span><u></u><u></u></p></div><blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm"><div><div><p><span lang="EN-GB">>> In general, associating comments is hard and it seems reasonable to me either way -- including or excluding.</span><u></u><u></u></p><p><span lang="EN-GB">Sure, as long as it’s consistent. The semicolon (and preceding comments) are not.</span><u></u><u></u></p><p><span lang="EN-GB"> </span><u></u><u></u></p><p><span lang="EN-GB">>> it infects other statements as well</span><u></u><u></u></p><p><span lang="EN-GB">Yeah. I realized that too late…</span><u></u><u></u></p><p><span lang="EN-GB">I’ve added a makeUnifiedFileCharRange function to LexerUtils which basically simply calls findTokenSkippingComments + some special handling for NullStmt.</span><u></u><u></u></p><p><span lang="EN-GB">I’ve tried token by token, but I cannot get this to work without a “full blown” parser as there is a very close match to consider:</span><u></u><u></u></p><p><span lang="EN-GB">- In the first case “end while” obviously belongs to the while statement and there is no semicolon to search for. I have to take tok::r_brace as an end. It’s also impossible to go for isa<WhileStmt> since it is nested.</span><u></u><u></u></p><p><span lang="EN-GB">- In the second case “assign” obviously belongs to the statement. Here I have to look beyond tok::r_brace and not stop there.</span><u></u><u></u></p><p><span lang="EN-GB"> </span><u></u><u></u></p><p><span lang="EN-GB">Any advice?</span><u></u><u></u></p></div></div></blockquote><div><div><p class="MsoNormal"><span style="font-family:Arial,sans-serif">Have you looked at the code I pointed to earlier (getAssociatedRange)? Properly handling those kinds of cases is quite difficult, but I don't think you need to parse the code. It's more about newlines and such.<u></u><u></u></span></p></div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal"><u></u> <u></u></p></div><div><p class="MsoNormal"> <u></u><u></u></p></div><blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm"><div><div><p><span lang="EN-GB"> </span><u></u><u></u></p><p><span lang="EN-GB"> </span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(106,153,85)">// RUN: %check_clang_tidy %s readability-braces-around-statements %t</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> </span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">int</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(220,220,170)">foo</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">() { </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(197,134,192)">return</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(181,206,168)">42</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">; }</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> </span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">void</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(220,220,170)">test</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">() {</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">  </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(197,134,192)">if</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> (</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">true</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">)</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">    </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(197,134,192)">if</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> (</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">true</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">)</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">      </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(197,134,192)">while</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> (</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(206,145,120)">"ok"</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">) {</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">        </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(220,220,170)">foo</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">();</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">      }</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(106,153,85)"> // end while</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> </span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">  </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">int</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> s;</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">  </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(197,134,192)">if</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> (</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">true</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">)</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">    </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(197,134,192)">if</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)"> (</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">true</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">)</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">      s = </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(86,156,214)">int</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">{</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">        </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(220,220,170)">foo</span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">()</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">      }; </span><span lang="EN-GB" style="font-size:10.5pt;font-family:Consolas;color:rgb(106,153,85)">// assign</span><u></u><u></u></p><p class="MsoNormal" style="line-height:14.25pt"><span style="font-size:10.5pt;font-family:Consolas;color:rgb(212,212,212)">}</span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB"> </span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB"> </span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB">Best regards,</span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB">Alexander</span><u></u><u></u></p><p class="MsoNormal"><span lang="EN-GB"> </span><u></u><u></u></p><div><blockquote style="border-top:none;border-right:none;border-bottom:none;border-left:1pt solid rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin:5pt 0cm 5pt 4.8pt"><div><div><p class="MsoNormal"><span lang="EN-GB"> </span><u></u><u></u></p></div></div></blockquote></div></div></div></blockquote></div></div></div></div></blockquote></div>