On May 15, 2012, at 11:57 AM, David Blaikie <[email protected]> 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 <[email protected]> wrote: >> >> 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? > > 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. >>> 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. > > It is convenient, certainly. Though I find op* returning T& to be > convenient/useful as well. In many cases we assign *i to a local > pointer (I'm not sure why - perhaps we should just have a better name > for the iterator and use that directly) & don't get the convenience of > op-> (I assume this code was written before the op-> was added, > actually) let alone the convenience of just using '.'. > > But if that's what you want, I can do that. "I.foo" vs. "I->foo" doesn't matter much to me, but "(*I)->foo" and "&*I" are really, really ugly. >> 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>). > > That's fine - though, since I want to replace > specific/filtered_decl_iterators with some generic adapters, those > adapters will need to provide the op-> convenience you want, whether > or not it was specified in the concept/standard. Yeah. >>> 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. > > For the basic decl_iterators they do (because the underlying sequence > is Decl*) but for the specific/filtered, where it'd be SpecificDecl** > but we don't have a real SpecificDecl* to point to, it wouldn't and > I'd need a proxy. Are there other cases/reasons for needing a proxy > that you're referring to? I don't know of other cases, no. >>> 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. > > I'm fairly sure all 3 of these should be consistent, and that the > specific/filtered should be refactored into a common adapter > (typedef'd in place so clients of Decl wouldn't need to change, of > course). Generally I prefer references, but I realize they're not a > perfect fit. > > Yeah - I'd like to be able to get more discussion on this & other > stylistic (micro design?) aspects - I kind of feel like I'm just > throwing stuff up & seeing what's stuck after a few weeks. IRC is generally the best place for micro-design discussions. - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
