Ticket #1682 (closed RFC: fixed)

Opened 4 years ago

Last modified 3 years ago

VTABLE_substr is never used

Reported by: NotFound Owned by: whiteknight
Priority: minor Milestone:
Component: core Version: 2.5.0
Severity: low Keywords: gci
Cc: Language:
Patch status: new Platform:

Description

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.

Attachments

tt1682.diff Download (8.0 KB) - added by jkeenan 3 years ago.
Eliminate VTABLE_substr; rename substr_str to substr

Change History

  Changed 4 years ago by bacek

+1 to remove VTABLE_substr

  Changed 4 years ago by 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?

  Changed 4 years ago by cotto

  • keywords gci added

  Changed 4 years ago by 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.

follow-up: ↓ 6   Changed 4 years ago by 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.

in reply to: ↑ 5   Changed 3 years ago by 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?

  Changed 3 years ago 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.

  Changed 3 years ago by 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

  Changed 3 years ago by dukeleto

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

  Changed 3 years ago by jkeenan

  • owner set to jkeenan
  • status changed from new to assigned
  • component changed from none to core

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

Changed 3 years ago by jkeenan

Eliminate VTABLE_substr; rename substr_str to substr

  Changed 3 years ago by jkeenan

  • patch set to new

follow-up: ↓ 13   Changed 3 years ago by 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

in reply to: ↑ 12   Changed 3 years ago by jkeenan

Replying to jkeenan:

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

Repeat request.

  Changed 3 years ago by whiteknight

  • owner changed from jkeenan to whiteknight
  • status changed from assigned to new

  Changed 3 years ago by jkeenan

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

Thank you very much.

kid51

  Changed 3 years ago by whiteknight

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

This is done. Closing ticket.

Note: See TracTickets for help on using tickets.