[llvm] r175847 - Code cleanup: pass Offset by pointer to parseInstruction to more explicitly

[David Blaikie]

On Fri, Feb 22, 2013 at 8:28 AM, Eli Bendersky <eliben at google.com> wrote:

> On Thu, Feb 21, 2013 at 9:58 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Thu, Feb 21, 2013 at 4:50 PM, Eli Bendersky <eliben at google.com>
> wrote:
> >>
> >> Author: eliben
> >> Date: Thu Feb 21 18:50:48 2013
> >> New Revision: 175847
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=175847&view=rev
> >> Log:
> >> Code cleanup: pass Offset by pointer to parseInstruction to more
> >> explicitly
> >> convey that it's a INOUT argument.
> >
> >
> > This isn't really a convention we have in the LLVM code base & it's not
> one
> > I'd personally prefer we adopt (though my opinion isn't the only one, of
> > course). Mostly because I always wonder whether such a parameter can be
> null
> > (from both the caller & the callee) and what it means if it is.
> >
> > Is there some other way we could tweak such an API to communicate this?
> > Though honestly I've seen enough parse/cursor APIs that this doesn't seem
> > /that/ unusual/confusing. Was there a particularly confusing call site
> you
> > saw?
> >
>
> This is mainly bringing the code in line with the other DataExtractor
> use cases. DataExtractor's getXXX methods take a pointer to offset for
> reading it and updating it. My initial code used a reference, and I
> wanted it to be more consistent now. As for the coding convention, I
> know we don't have an explicit guideline for this, and yet I
> personally prefer not to use references in such cases, because looking
> at:
>
> doFoo(param)
>
> It's really hard to immediately see that param can be modified by
> doFoo. But looking at:
>
> doFoo(&param)
>
> It's easier.
>
> But this is a minor argument. The main one is the consistency.


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...

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)

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130222/25e2c54d/attachment.html>