Ticket #1449 (closed todo: fixed)

Opened 4 years ago

Last modified 4 years ago

Add support for String PMC in NameSpace::get_pmc_keyed

Reported by: Austin_Hastings Owned by:
Priority: normal Milestone:
Component: none Version: 2.0.0
Severity: medium Keywords:
Cc: Language:
Patch status: Platform:

Description

Presently, NameSpace PMCs support keyed_pmc access only for Key PMC types:

austin@andLinux:~/kakapo$ cat test.pir
.sub main
    $P1 = get_hll_namespace

    say "Testing with Key"
    $P0 = new 'Key'
    $P0 = 'Foo'
    $P2 = $P1[$P0]                # OKAY

    say "Testing with String PMC"
    $P0 = new 'String'
    $P0 = 'Foo'
    $P2 = $P1[$P0]                # FAIL
.end
austin@andLinux:~/kakapo$ parrot test.pir
Testing with Key
Testing with String PMC
Invalid namespace key in get_pmc_keyed_str
current instr.: 'main' pc 22 (test.pir:12)

Given than NameSpace supports keyed access with keyed_str, it should also support String PMCs.

Change History

  Changed 4 years ago by whiteknight

I created the branch tt_1449 to look into this issue and committed a potential fix in r44021. My fix was to allow ANY type as a potential key, so long as it responds to the VTABLE_get_string call. Only a Key PMC can be used to obtain nested NameSpaces.

If this fix is correct, we need to add tests for the new behavior, add a node in DEPRECATED.pod about the potentially experimental status of the behavior, and merge the branch to trunk after the 2.1 release.

follow-up: ↓ 6   Changed 4 years ago by NotFound

I think that allowing String arrays for nested namespaces may be useful. They are easier to create dynamically than Keys.

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

> Comment(by NotFound):
>
>  I think that allowing String arrays for nested namespaces may be useful.
>  They are easier to create dynamically than Keys.

If we support arrays in places where we currently require Keys, would
we need Keys at all anymore?

-- 
Will "Coke" Coleda

  Changed 4 years ago by NotFound

There was some talk about that some time ago, don't remember well. We need pir support for constant string arrays if we want to use it to replace current key usages.

in reply to: ↑ 3   Changed 4 years ago by whiteknight

Replying to coke:

If we support arrays in places where we currently require Keys, would we need Keys at all anymore?

Yes. Keys are really the only aggregate types we can define as constants in PIR code. There is no PIR syntax to support other types of constants, yet.

in reply to: ↑ 2 ; follow-up: ↓ 8   Changed 4 years ago by whiteknight

Replying to NotFound:

I think that allowing String arrays for nested namespaces may be useful. They are easier to create dynamically than Keys.

So howabout this for a final design:

  • For Key PMCs, follow current behavior to return a nested namespace
  • For PMCs that "does array", loop over VTABLE_get_string_keyed_int to return a nested namespace
  • For other types, use VTABLE_get_string to get a non-nested namespace.

Does this satisfy everybody's requirements?

  Changed 4 years ago by coke

> Comment(by whiteknight):
>
>  Replying to [comment:3 coke]:
>  > If we support arrays in places where we currently require Keys, would
>  > we need Keys at all anymore?
>
>  Yes. Keys are really the only aggregate types we can define as constants
>  in PIR code. There is no PIR syntax to support other types of constants,
>  yet.

If we don't need Keys, then the current syntax that generates Keys
could be taken over to generate an Array type.

-- 
Will "Coke" Coleda

in reply to: ↑ 6   Changed 4 years ago by NotFound

Replying to whiteknight:

So howabout this for a final design: - For Key PMCs, follow current behavior to return a nested namespace - For PMCs that "does array", loop over VTABLE_get_string_keyed_int to return a nested namespace - For other types, use VTABLE_get_string to get a non-nested namespace.

+1

  Changed 4 years ago by whiteknight

I've committed this new algorithm to the branch. Testing, including help writing test cases for the new behavior, would be much appreciated.

  Changed 4 years ago by whiteknight

  • status changed from new to closed
  • resolution set to fixed

Branch merged to trunk in r44104.

Note: See TracTickets for help on using tickets.