<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Feb 22, 2013 at 8:28 AM, Eli Bendersky <span dir="ltr"><<a href="mailto:eliben@google.com" target="_blank">eliben@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Thu, Feb 21, 2013 at 9:58 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>

><br>
><br>
><br>
> On Thu, Feb 21, 2013 at 4:50 PM, Eli Bendersky <<a href="mailto:eliben@google.com">eliben@google.com</a>> wrote:<br>
>><br>
>> Author: eliben<br>
>> Date: Thu Feb 21 18:50:48 2013<br>
>> New Revision: 175847<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=175847&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=175847&view=rev</a><br>
>> Log:<br>
>> Code cleanup: pass Offset by pointer to parseInstruction to more<br>
>> explicitly<br>
>> convey that it's a INOUT argument.<br>
><br>
><br>
> This isn't really a convention we have in the LLVM code base & it's not one<br>
> I'd personally prefer we adopt (though my opinion isn't the only one, of<br>
> course). Mostly because I always wonder whether such a parameter can be null<br>
> (from both the caller & the callee) and what it means if it is.<br>
><br>
> Is there some other way we could tweak such an API to communicate this?<br>
> Though honestly I've seen enough parse/cursor APIs that this doesn't seem<br>
> /that/ unusual/confusing. Was there a particularly confusing call site you<br>
> saw?<br>
><br>
<br>
</div>This is mainly bringing the code in line with the other DataExtractor<br>
use cases. DataExtractor's getXXX methods take a pointer to offset for<br>
reading it and updating it. My initial code used a reference, and I<br>
wanted it to be more consistent now. As for the coding convention, I<br>
know we don't have an explicit guideline for this, and yet I<br>
personally prefer not to use references in such cases, because looking<br>
at:<br>
<br>
doFoo(param)<br>
<br>
It's really hard to immediately see that param can be modified by<br>
doFoo. But looking at:<br>
<br>
doFoo(&param)<br>
<br>
It's easier.<br>
<br>
But this is a minor argument. The main one is the consistency.</blockquote><div><br></div><div style>Oh, OK - I'll totally buy an argument from consistency if the code you're being consistent with is nearby/obviously in contrast (ie: the two uses sit side by side & would confuse users - which I guess is the case here), but if we're both in agreement (a preference for reference in/out params) why not just fix DataExtractor instead (& that way it'll be consistent with the rest of the codebase)? It's not a huge class & the transformation would be fairly easy I would think... <br>
<br>But I'm not too fussed/rushed by this - just wanted to make sure we weren't making a general/broader habit (though the "consistency creep" could lead to that, which would be unfortunate - I'd rather creep in the other direction where convenient to do so)<br>
<br>- David</div></div><br></div></div>