<div class="gmail_quote"><div dir="ltr">On Thu, Sep 21, 2017, 17:46 Eric Christopher via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">echristo accepted this revision.<br>
echristo added a comment.<br>
<br>
Other than the readability issue, sgtm.<br>
<br>
-eric<br>
<br>
<br>
<br>
================<br>
Comment at: llvm/lib/CodeGen/XRayInstrumentation.cpp:205<br>
     // For the architectures which don't have a single return instruction<br>
-    prependRetWithPatchableExit(MF, TII);<br>
+    prependRetWithPatchableExit(MF, TII, {false, true});<br>
+    break;<br>
----------------<br>
timshen wrote:<br>
> dberris wrote:<br>
> > Looking at this, I'm inclined to suggest that using an `enum class` for spelling out whether to handle tail calls might be more readable. Right now I have to see what the meaning of 'false' in the first position is. Something like:<br>
> ><br>
> > ```<br>
> > prependRetWithPatchableExit(MF, TII, {InstrumentationOptions::TailCalls::IGNORE, InstrumentationOptions::Returns::HANDLE_ALL});<br>
> > ```<br>
> ><br>
> > The enum class may or may not be nested in InstrumentationOptions, so I'll leave that up to you.<br>
> FYI designated initializer (<a href="http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0329r4.pdf" rel="noreferrer" target="_blank">http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0329r4.pdf</a>) should be in C++20, so ultimately we'll have this:<br>
>   prependRetWithPatchableExit(MF, TII, {.HandleTailCalls = false, .HandleAllReturns = true});<br>
><br>
> but not now. :)<br>
><br>
> As for your suggested solution, I feel like the callee side code will be much longer and confusing, so I'll keep it as is.<br>
Bools are pretty terrible and while I agree that designated initializers would be better, named constants are at least better than false, true.<br>
<br>
<br>
================<br>
Comment at: llvm/lib/CodeGen/XRayInstrumentation.cpp:45<br>
+<br>
+  InstrumentationOptions(bool HandleTailcall, bool HandleAllReturns)<br>
+      : HandleTailcall(HandleTailcall), HandleAllReturns(HandleAllReturns) {}<br>
----------------<br>
How about an enum mask set?<br></blockquote></div><div><br></div><div>Enum values are default constructible, so it's not better than:</div><div>  InstrumentationOptions op;</div><div>  op.x = false;</div><div>  op.y = true;</div><div><br></div><div>Now I prefer going back to the default ctor + assignment style.</div><div><br></div><div>Data members don't have to have default values. If users forgot to assign values, ubsan will catch them.</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<a href="https://reviews.llvm.org/D38102" rel="noreferrer" target="_blank">https://reviews.llvm.org/D38102</a><br>
<br>
<br>
<br>
</blockquote></div>