VTABLE_substr is never used

[NotFound]

Checking code coverage I discovered that VTABLE_substr in the String PMC is uncovered. Looking at it, I've found that there is no place in the repository that uses it except generated code for the Object PMC. The substr opcodes use VTABLE_substr_str, not VTABLE_substr.

Is there some usage in HLL or extensions? If not, I suggest to drop it.

[bacek]

+1 to remove VTABLE_substr

[Paul C. Anagnostopoulos]

Might it make sense to make it synonymous with VTABLE_substr_str and deprecate VTABLE_substr_str? Why the redundant '_str' suffix?

[dukeleto]

It seems that VTABLE_substr takes a destination argument, whereas VTABLE_substr_str returns a value. Which behavior do we want?

Also, there are the Parrot_PMC_substr and Parrot_PMC_substr_str functions in extend_vtable.c that use these.

[cotto]

Unless there's a user who cares, let's deprecate and drop the current usage of the substr VTABLE slot and rename substr_str to substr.

[jkeenan]

Replying to cotto:

Unless there's a user who cares, let's deprecate and drop the current usage of the substr VTABLE slot and rename substr_str to substr.

Do we have an agreement on a deprecation here? If so, what is our recovery path? Who can take this on?

[whiteknight]

I'm in favor of this, and so far I haven't seen any objections. We should double check with the Rakudo folks that they don't need this, and kill it if not.

[pmichaud]

On Mon, May 23, 2011 at 06:42:19PM -0000, Parrot wrote:
> Comment(by whiteknight):
> 
>  I'm in favor of this, and so far I haven't seen any objections. We should
>  double check with the Rakudo folks that they don't need this, and kill it
>  if not.

A quick check of Rakudo's sources confirms that we're not making
use of VTABLE_substr anywhere.  I don't know if that means that other HLLs
wouldn't someday need it, but at present we don't.

Pm

[dukeleto]

There are tests for these currently in t/src/extend_vtable.t, but feel free to modify or remove them.

[jkeenan]

I created the tt1682/vtable_substr branch in it to work on this ticket. I am attaching a patch, tt1682.diff, which shows the changes made so far in this branch.

kid51

[jkeenan]

Eliminate VTABLE_substr; rename substr_str to substr

[jkeenan]

Could we get the patch reviewed by people who have contributed to this ticket?

Since it was created mostly by search-and-replace, with no particular knowledge of VTABLESs or substr, it needs careful review before we proceed with deprecation.

Thank you very much.

kid51

[jkeenan]

Replying to jkeenan:

Could we get the patch reviewed by people who have contributed to this ticket?

Repeat request.

[jkeenan]

whiteknight: With a branch merge into master today, is this ticket closable?

Thank you very much.

kid51

[whiteknight]

This is done. Closing ticket.