<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">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>
<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 class="h5"><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><br></div><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><br></div><div>Maybe something like:</div><div><br></div><div>enum {</div><div>  Success,</div>
<div>  Failure</div><div>};</div><div>or</div><div><div>enum {</div><div>  Failure,</div><div>  Success</div><div>};</div></div><div><br></div><div>-- Sean Silva</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">
<div style="word-wrap:break-word"><span class=""><font color="#888888"><div><br></div><div>-Nick</div><div><br></div><div><br></div></font></span></div>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a>         <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br></div></div>