On May 15, 2012, at 9:31 AM, David Blaikie <[email protected]> wrote:
> On Tue, May 15, 2012 at 9:19 AM, Douglas Gregor <[email protected]> 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? We tend use pointers (not references) for AST nodes throughout AST, Sema, and a number of other places in the compiler. > This means killing off the op-> for these iterators though, right? Or > do you want to keep that as an excusable convenience? Honestly, I preferred the world back in its conveniently-inconsistent state where operator* returned a pointer and operator-> returned that same pointer, because being able to refer to Iter->getDeclName() rather than (*Iter)->getDeclName() is *really* convenient. The inclusion of operator-> in the iterator concepts was a mistake. Nothing in the standard library actually depends on it, no sane algorithms need it, and it's a major PITA to implement for iterator adaptors (as you've noticed <g>). > In which case my > change boils down to just changing the pointer typedefs to be Decl** > in these various iterator types. (& might make my planned work, > generalizing the filtering iterators, a bit weird - possibly providing > an op-> to the generic filtering iterators whenever the value type is > a pointer to match/support this oddity) Decl** won't always work, though. You'll almost certainly need a proxy. > Would you prefer the other iterators I changed > (specific_decl_iterator, filtered_decl_iterator) be switched to > pointer value_type too? I did prefer the pointer value_type; perhaps others want to weigh in. >> This isn't going to work; m_func returns a value_type or reference. Dealing >> with -> is a real pain, because in the general case you need a proxy class. >> >> +// a convenient utility for a common adaptation >> +template<typename T> >> +struct to_pointer { >> + typedef T * result_type; >> + typedef T argument_type; >> + result_type &operator()(T &t) const { >> + ptr = &t; >> + return ptr; >> + } >> +private: >> + mutable T *ptr; >> +}; >> >> 'to_pointer' is a very non-LLVM-y name. >> >> More generally, this particular function object seems like a bad idea. >> There's nothing wrong with having an iterator type whose reference type is >> not a true reference, which means that there is no reason to do this little >> dance with the mutable 'ptr'. > > Yeah, a proxy would be nicer. > > In any case all this transformation stuff isn't needed for this change > anymore if we're sticking with a pointer value_type. Right. - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
