<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 29, 2017 at 5:18 PM David Li 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">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><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><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/D34838</a><br>
<br>
<br>
<br>
</blockquote></div></div>