[cfe-commits] [patch] TransformIterator (motivating use case in Clang)

[Chandler Carruth]

On Tue, May 15, 2012 at 3:11 PM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On May 15, 2012, at 11:57 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
> > [-llvm-commits since it's not too relevant to that list at this point]
> >
> > On Tue, May 15, 2012 at 11:24 AM, Douglas Gregor <dgregor at apple.com>
> wrote:
> >>
> >> On May 15, 2012, at 9:31 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >>
> >>> On Tue, May 15, 2012 at 9:19 AM, Douglas Gregor <dgregor at apple.com>
> wrote:
> >>>>
> >>>> On May 6, 2012, at 7:02 PM, David Blaikie wrote:
> >>>>
> >>>>> This patch adds an iterator much like boost's transform_iterator
> >>>>> (without some extra bells & whistles) for use in some changes I'd
> like
> >>>>> to make to Clang to correct some misimplemented iterators there.
> >>>>>
> >>>>> A few gotchas that could be improved/changed depending on opinions:
> >>>>> * I may be playing a little loose with the return type of the functor
> >>>>> (see the example/motivating functor, to_pointer) - the return type
> >>>>> actually must be a reference, though the result_type provides the
> >>>>> underlying value type, not the reference type. If this is violated
> >>>>> Clang will emit a warning, but I could make it more robust with a
> >>>>> compile time assertion in the TransformIterator that the result_type
> >>>>> is actually a reference type, and strip that off to provide the
> >>>>> value_type of the TransformIterator.
> >>>>
> >>>> It's much more correct for the value and reference types of the
> iterator type to be, e.g.,
> >>>>
> >>>>  typedef typename Functor::result_type reference;
> >>>>  typedef typename remove_reference<reference>::type value_type;
> >>>>
> >>>>> * I realize adding pointer-yness back onto an iterator over
> references
> >>>>> when the underlying data structure (in the Clang patch you can see
> >>>>> this situation) is a sequence of pointers may be a bit overkill -
> >>>>> though the alternatives are writing different algorithms in the two
> >>>>> cases where I've used TransformIterator, or having the decl_iterator
> >>>>> iterate over pointers (in which case I should probably change the
> >>>>> other iterators I've modified to iterate over pointers for symmetry).
> >>>>
> >>>> diff --git include/clang/AST/DeclBase.h include/clang/AST/DeclBase.h
> >>>> index 6aef681..0a16ea5 100644
> >>>> --- include/clang/AST/DeclBase.h
> >>>> +++ include/clang/AST/DeclBase.h
> >>>> @@ -1175,17 +1175,18 @@ public:
> >>>>     Decl *Current;
> >>>>
> >>>>   public:
> >>>> -    typedef Decl*                     value_type;
> >>>> -    typedef Decl*                     reference;
> >>>> -    typedef Decl*                     pointer;
> >>>> +    typedef Decl                     value_type;
> >>>> +    typedef value_type&              reference;
> >>>> +    typedef value_type*              pointer;
> >>>>
> >>>> Since we tend to traffic in declaration pointers, not references,
> it's really beneficial to have value_type be Decl*.
> >>>
> >>> Fair enough - I was hoping to reduce the amount of pointers handed
> >>> around, but I realize in this case especially (where the underlying
> >>> sequence is a sequence of pointers) it's really an awkward fit.
> >>
> >> Why is reducing the number of pointers a specific goal?
> >
> > Evidently not something everyone finds to be a useful goal - and on
> > that basis perhaps I should just leave it alone.
> >
> > But for myself I find functions taking references (& locals that are
> > references) easier to read/reason about because it's clear that
> > they're not null. Trying to decide which functions are accepting
> > optional values and which ones aren't makes me pause a little - though
> > perhaps more out of habit than real curiosity about whether something
> > is null. If switching things to references causes pointer-habitual
> > people to have the same kind of pause then that's clearly not an
> > outright improvement.
>
> Unless we're seeing lots of problems due to the pointer-centric style, I
> don't see a motivation to make this kind of change in the code base.


Since there was some mention of wanting other perspectives here, I wanted
to chime in with my two cents...

This seems like a lot of work and a disruptive change in API patterning,
and I'm really not clear what problem it's trying to solve. My suspicion is
that there isn't one other than a preference for references instead of
pointers.

I actually don't even agree with this movement. While yes, it is nice to
use simpler constructs when there is no possibility of a null pointer,
references aren't strictly superior. We can't assign to them to re-point
them to other entities, which is a fundamental pattern in Clang and LLVM. I
don't see soft "non-null" constraints as a sufficient reason to lose this.

What's more, it is trivial to add non-null constraints to pointers. In
fact, dyn_cast/cast/isa all provide such constraints. We could teach Clang
about [[nonnull]] for parameters and get much more than a reference gives
us:

- Optimizing based on non-null-ness
- Checking in non-optimized builds for non-null-ness
- Still able to overwrite the argument within the funciton
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120515/8c7f19ab/attachment.html>