Ticket #811 (closed RFC: done)

Opened 5 years ago

Last modified 5 years ago

[RFC] Deprecate "new Iterator" form for creating Iterators.

Reported by: bacek Owned by: bacek
Priority: normal Milestone:
Component: core Version: 1.3.0
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

Hello.

Currently there is 2 ways of creating iterators:

1. $P0 = new 'Iterator', aggregate

2. $P0 = get_iter aggregate

As part of keys_revamp branch I start creating aggregate-specific iterators. E.g. HashIterator, ArrayIterator, etc. Which leads to next code in Iterator PMC:

    VTABLE void init_pmc(PMC *aggregate) {
        if (VTABLE_does(INTERP, aggregate, CONST_STRING(INTERP, "array"))
            || VTABLE_does(INTERP, aggregate, CONST_STRING(INTERP, "hash"))
            || VTABLE_does(INTERP, aggregate, CONST_STRING(INTERP, "string"))
            ) {
            /* It's ugly hack... But I cant figure out proper way to do it. */
            PMC *real_iter = VTABLE_get_iter(INTERP, aggregate);
            SELF = pmc_reuse_init(INTERP, SELF, VTABLE_type(INTERP, real_iter), aggregate, 0);
            return;
        }
        else {
            /* Die horribly */
            PARROT_ASSERT(!"Unsupported Aggregate for Iterator");
        }
    }

Main purpose of Iterator.init is create real iterator and "morph" to it.

It's error-prone, inconsistent and ugly.

My propose is to deprecate "new Iterator, aggregate" form of creating iterators and left only VTABLE-based "get_iter" op.

-- Bacek

Change History

in reply to: ↑ description   Changed 5 years ago by pmichaud

Replying to bacek:

Currently there is 2 ways of creating iterators: 1. $P0 = new 'Iterator', aggregate 2. $P0 = get_iter aggregate

Minor correction: the opcode is "iter", not "get_iter".

My propose is to deprecate "new Iterator, aggregate" form of creating iterators and left only VTABLE-based "get_iter" op.

+1 for using the "iter" opcode as the primary mechanism for getting iterators. I have no strong opinion for/against deprecating "new 'Iterator', aggregate".

Pm

follow-up: ↓ 3   Changed 5 years ago by coke

  • owner set to bacek

Recent commits to stop "new 'Iterator'" have broken examples_tests.

in reply to: ↑ 2   Changed 5 years ago by bacek

Replying to coke:

Recent commits to stop "new 'Iterator'" have broken examples_tests.

Ooops. Sorry. Fixed in r40199 and r40200.

-- Bacek

  Changed 5 years ago by bacek

  • status changed from new to closed
  • resolution set to done
Note: See TracTickets for help on using tickets.