<div dir="ltr">On 8 January 2013 14:14, Tim Northover <span dir="ltr"><<a href="mailto:Tim.Northover@arm.com" target="_blank">Tim.Northover@arm.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 class="im">I've got some updated patches here that remove any default behaviour; as<br>
</div>
Duncan pointed out it *is* largely inappropriate. The new interface is<br>
<br>
    APFloat::APFloat(const fltSemantics&, const APInt&)<br></blockquote><div><br></div><div style>I agree both alternatives were worse than this. I think it turned out rather cleaner than I expected.</div><div><br>
</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">On the whole it turned out rather neatly. When needed, the fltSemantics were<br>

usually easy to come by, even though there's a redundancy in APInt size and<br>
semantics for the smaller cases.<br></blockquote><div><br></div><div style>It looks better now, indeed, including the removal of the redundant static EVTToAPFloatSemantics().</div><div style><br></div><div style>The LLVM patch looks good to me.</div>
<div style><br></div><div style>On the Clang patch:</div><div style><br></div><div style><div>+enum APFloatSemantics {</div><div>+  IEEEhalf,</div><div>+  IEEEsingle,</div><div>+  IEEEdouble,</div><div>+  x87DoubleExtended,</div>
<div>+  IEEEquad,</div><div>+  PPCDoubleDouble</div><div>+};</div><div><br></div><div style>I not sure what would be the best approach. It seems redundant and error prone to have this here...</div><div style><br></div><div style>
Having the same enum on APFloat.h would only be marginally better, though.</div><div style><br></div><div style><div>-  return llvm::APFloat(ReadAPInt(Record, Idx));</div><div>+  APFloatSemantics Semantics = static_cast<APFloatSemantics>(Record[Idx++]);</div>
<div><br></div><div style>Maybe this is better to be in a function itself, say ReadAPFloat().</div></div><div style><br></div><div style>How does the producer of Record knows about that enum in the first place? Seems odd that you just magically assumed both parts are agreeing to the same interface when the produce is not using it... Or I got it completely wrong, quite likely... ;)</div>
<div style><br></div><div style>cheers,</div><div style>--renato</div></div></div></div></div>