<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 29, 2017 at 5:29 PM, 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><br><div class="gmail_quote"><span class=""><div dir="ltr">On Thu, Jun 29, 2017 at 5:18 PM David Li via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">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">davidxl added a comment.<br>
<br>
Instead of exposing Counter members widely (and fixing tests), it is better to add wrapper methods in InstrProfRecord to forward the call to the nested member struct.<br></blockquote></span><div><br>Seems like a lot of wrappers - is that the ideal API going forward, or only to minimize changes made by this patch? (I've already done the work & LLVM as a community's usually fine with the churn such that minimizing change isn't too important if it seems like the way things should be)<br><br>Though I admit this is a bit so-so/uncertain. One pro is that I think lacking the wrappers encourages APIs to only depend on the part of the structure they need (in fact on that basis I'd consider sinking many of the value-profiling based functions into a separate class wrapping the unique_ptr) rather than the whole thing, making their contract clearer ("this function is only dealing with value profiling data", "this function cares about the counters as well", "this function needs names, etc") & more flexible (you can call it when you only have that part/chunk/thing, not needing the whole thing around).<br></div></div></div></blockquote><div><br></div><div>Exposing the internals of the data structure in interfaces make future data structure changes more difficult.  I understand there are quite a few wrappers, but they are mostly just single-liners. With the wrappers available, I expect most of the client side code remain unchanged (other than the writer code).</div><div><br></div><div>David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><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/D34838" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D34838</a><br>
<br>
<br>
<br>
</blockquote></div></div>
</blockquote></div><br></div></div>