<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Mar 23, 2015 at 11:49 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I am actually not sure how clang-tidy should handle cases like this (where two checks would fix the same code in different ways) and it's probably something we should figure out in the long run.<div><br></div><div>In this case, the else after return (yes, we do have a check for that although it doesn't yet offer automatic fixing) should take precedence as there is an explicit rule in the coding standards.</div><div><br></div><div>Of course, that still leaves the question of whether clang-tidy should also simplify:</div><div><br></div><div>  if (a)</div><div>    return true;</div><div>  return false;</div><div><br></div><div>to</div><div><br></div><div>  return a;</div></div></blockquote><div><br>Yep - if/when the check gets to catching this (which it seems it should - there's nothing fundamentally 'better' about this code than the other code) then John's issue's apply again and I'd be inclined to agree with him - though the exact nature of the suppression, I'm not sure - possibly suppress the warning if the 'if' is an else and the prior if returns on all paths? Dunno.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 7:41 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Mar 23, 2015 at 11:30 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I don't feel strongly about this, and I can see some of your reasoning. However, an "if (a) return true; else return false;" is very suspect to me and I think "return a;" is more readable, independent of whether it is at the end of a chain or not.<div><br></div><div>However, there is an underlying issue here. The existing code is violating LLVM's coding standard of "don't use else after return" (<a href="http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return" target="_blank">http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return</a>). So in these chains, all of the "else"s should be removed.</div></div></blockquote></span><div><br>& that incidentally (sounds like more by accident than design) suppresses the warning. Though, arguably, this clang-tidy check perhaps shouldn't be passing judgment on that particular case and we should have an else-after-return check (we probably already do) for that explicitly.<br><br>That said, I don't much mind this check happening to provide else-after-return catching in this way. (which is to say, in a round about way, I agree with you Daniel) Maybe it's just a case of two different style checkers happening to catch the same code for different reasons, and the same potential code change addresses both of them.<br> </div><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 7:17 AM, Richard <span dir="ltr"><<a href="mailto:legalize@xmission.com" target="_blank">legalize@xmission.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks for your ideas, John, I'm going to incorporate this into the `clang-tidy` check.<br>
<div><div><br>
<br>
<a href="http://reviews.llvm.org/D8532" target="_blank">http://reviews.llvm.org/D8532</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>