<div dir="ltr">On Tue, Sep 24, 2013 at 4:44 PM, Nick Kledzik <span dir="ltr"><<a href="mailto:kledzik@apple.com" target="_blank">kledzik@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div class="im"><div>On Sep 24, 2013, at 4:07 PM, Sean Silva <<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">On Tue, Sep 24, 2013 at 4:17 PM, Nick Kledzik <span dir="ltr"><<a href="mailto:kledzik@apple.com" target="_blank">kledzik@apple.com</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote">
<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 style="word-wrap:break-word"><div><div><br>
<div><div>On Sep 24, 2013, at 12:40 PM, Michael Spencer <<a href="mailto:bigcheesegs@gmail.com" target="_blank">bigcheesegs@gmail.com</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra">
<div>On Tue, Sep 24, 2013 at 12:33 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br></div><div class="gmail_quote">
<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 dir="ltr"><div>Hi LLD developers,</div><div><br></div>
<div>I'm about to make a change to invert the return value of Driver::parse() to return true on success. Currently it returns false on success.</div><div><br></div>
<div>In many other functions, we return true to indicate success and false to indicate failure. The inconsistency is confusing, and fixing it should improve code readability.</div><div><br></div></div></blockquote><div><br>
</div><div>Note that some places in LLVM use false to indicate success, not sure how widespread this is. Personally I think that { if (doSomething()) } means if doSomething succeeded, and thus agree with you. However, I think this is something that needs to be consistent across all of LLVM and should be in the coding standard.</div>
</div></div></div></blockquote><br></div></div></div><div>StringRef::getAsInteger() is an example documented to return true if there was an error parsing the string. </div></div></blockquote><div><br></div><div>I think it makes a lot of sense in this case. The idea is that you increase indentation in the "error" case. Consider parsing 2 numbers and returning their sum:</div>
<div><br></div><div>if (S1.getAsInteger(16, Result1)) {</div><div>// report failure</div><div>}</div><div>if (S2.getAsInteger(16, Result2)) {</div><div>// report failure<br>}</div><div>return Result1 + Result2;</div><div>
<br></div><div>vs.</div><div><br></div><div><div>if (S1.getAsIntegerTrueOnSuccess(16, Result1)) {</div><div> if (S2.getAsIntegerTrueOnSuccess(16, Result2)) {</div><div> return Result1 + Result2;</div><div> } else {</div>
<div> // report failure</div><div> }</div><div>} else {</div><div> // report failure</div><div>}</div></div></div></div></div></blockquote></div><div>Yes. And that is the case where lld was return true for errors. </div>
</div></div></blockquote><div><br></div><div>It's not very accurate to say that LLD was return true for errors. Some parts of LLD return true for errors, while other parts returns false.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div class="im"><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Of course, in the latter case you would just use ! to avoid gaining indentation, but still, I think that if most uses of an API will require immediately negating the return value, then maybe the sense should be inverted.</div>
<div> <br></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"><div style="word-wrap:break-word"><div><br>
</div><div>Mapping success/error onto a bool will always be ambiguous. Is there some better pattern?</div></div></blockquote></div></div></div></blockquote></div>Perhaps returning ErrorOr<void> instead of a bool would make it clearer. In practice that would mean a ! in the if statement, which runs counter to your strawman that if every client uses ! that the sense should be flipped, e.g.:</div>
<div> if (!parse(xx))</div><div> // handle error</div><div><br></div><div>or that test for error could be made obvious via:</div><div><br></div><div><div> if (error_code ec = parse(xx))</div><div> // handle error</div>
</div></div></blockquote><div><br></div><div>I don't think that ErrorOr<void> would really work well. "ec" would only be used in this "if" condition expression because it has no value, so the compiler would warn that "ec" is unused. We would have to write "if (parse(xx))", which is the same code as the original but is probably a bit more confusing.</div>
</div></div></div>