[cfe-commits] [possible patches] specific/filtered_decl_iterator aren't iterators

[David Blaikie]

[oops, meant to move this to cfe-commits in the process]

On Sun, Apr 29, 2012 at 5:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
> Well, in the interests of demonstration I've implemented a basic pass
> at both options. I wouldn't mind someone signing off on one of these
> two - the current state isn't really viable so far as I can see, so
> it's one or the other. I personally tend towards the "reference"
> version (& in the ref_further you can see I got bored & pushed
> references further down through a few other APIs that don't need the
> optionality (nullness) of pointers anyway) but I'm not terribly fussed
> either way.
>
> Thanks,
> - David
>
> On Tue, Apr 17, 2012 at 10:34 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> Bump.
>>
>> On Wed, Apr 4, 2012 at 2:28 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>> Resending to the intended (dev, rather than commits) mailing list.
>>>
>>> On Fri, Mar 30, 2012 at 12:54 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>> > These filtering iterators ( include/clang/AST/DeclBase.h:1304, 1226 (&
>>> > even decl_iterator at 1172 and redecl_iterator at 689)) aren't
>>> > actually implementing the iterator concept correctly. Most noticeably
>>> > their value, pointer, and reference types are all the same.
>>> >
>>> > The reason I'm interested in this is that I'd like to pull out some of
>>> > this functionality (filtering iterators*) so it can be reused by my
>>> > work improving the CFG to improve the accuracy of -Wunreachable-code.
>>> >
>>> > There seem to be two possible solutions to this:
>>> >
>>> > 1) value_type is SpecificDecl*
>>> > Not a perfect fit, because we don't have a SpecificDecl* available to
>>> > refer to in the case of the specific/filter iterators (since the real
>>> > pointer is a Decl*, not a SpecificDecl*). We could just keep a
>>> > temporary SpecificDecl* as a member of the iterator (I've a simple
>>> > prototype of this). This does mean removing the operator-> from these
>>> > iterators, since they don't iterate over struct types. Some code seems
>>> > to have been written with this in mind already ("(*iter)->foo") but
>>> > other parts (lots of uses of specific_decl_iterator, few of
>>> > filtered_decl_iterator) do use "iter->foo" - I guess the op-> was
>>> > added as an afterthought.
>>> >
>>> > 2) value_type is SpecificDecl
>>> > Also not a perfect fit, since we don't have concrete SpecificDecls -
>>> > we have pointers to possibly-derived instances of SpecificDecl, so if
>>> > anyone attempted "value_type v = *iter;" they would slice the object
>>> > (but I think we suppress the copy constructors anyway - so this is
>>> > perhaps not an issue). Other than that this seems to make the most
>>> > sense to me, since the Decl pointers cannot be null, so it's just
>>> > convenient to use '->' (or op* to get a SpecificDecl& directly, rather
>>> > than a SpecificDecl* that's always non-null anyway).
>>> > Actually this option may require/benefit from the use of adapting
>>> > iterators at the same time - to make it easy to copy the sequence of
>>> > pointers into a SmallVector which is done in a couple of places in
>>> > SemaLambda.
>>> >
>>> > I'd like to do (2) to at least filter/specific_decl_iterator (and
>>> > happy to fix up the other iterators with this inconsistency as well) &
>>> > then continue on to refactor out the functionality in to some general
>>> > purpose iterator adaptors for reuse in the CFG.
>>> >
>>> > Does that sound reasonable?
>>> >
>>> > * the filtering/specific iterators in DeclBase.h are a bit more than
>>> > the classic filter_iterator since they modify the value_type whereas a
>>> > filter_iterator (ala boost's implementation, for example) is only
>>> > about selecting on a condition. We'd need an adapting_iterator too, or
>>> > a single device combining both features of adapting and filtering.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: specific_iter_pointer.diff
Type: application/octet-stream
Size: 64575 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120429/299e0a0c/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: specific_iter_ref_further.diff
Type: application/octet-stream
Size: 159454 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120429/299e0a0c/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: specific_iter_ref.diff
Type: application/octet-stream
Size: 107830 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120429/299e0a0c/attachment-0002.obj>