Ticket #1785 (closed deprecation: fixed)

Opened 4 years ago

Last modified 4 years ago

src/pmc/oplib.pmc, src/pmc/resizeablestringarray.pmc: Deprecate duplicated VTABLE functions in

Reported by: jkeenan Owned by: jkeenan
Priority: normal Milestone: 2.10
Component: core Version: 2.7.0
Severity: medium Keywords:
Cc: pmichaud NotFound Language:
Patch status: applied Platform:

Description

In r48932, NotFound added a long overdue check for duplicated VTABLE functions -- a check which, when applied, turned up these two duplicates:

Index: /trunk/src/pmc/oplib.pmc
===================================================================
--- /trunk/src/pmc/oplib.pmc	(revision 48923)
+++ /trunk/src/pmc/oplib.pmc	(revision 48932)
@@ -117,8 +117,4 @@
     }
 
-    VTABLE INTVAL get_integer() {
-        return STATICSELF.elements();
-    }
-
     METHOD op_family(STRING *shortname)
     {
Index: /trunk/src/pmc/resizablestringarray.pmc
===================================================================
--- /trunk/src/pmc/resizablestringarray.pmc	(revision 46300)
+++ /trunk/src/pmc/resizablestringarray.pmc	(revision 48932)
@@ -303,23 +303,4 @@
 /*
 
-=item C<PMC *shift_pmc()>
-
-Removes and returns the first element in the array.
-
-=cut
-
-*/
-
-    VTABLE PMC *shift_pmc() {
-        STRING * const strval = SELF.shift_string();
-        PMC    * const value  = Parrot_pmc_new(INTERP, enum_class_String);
-
-        VTABLE_set_string_native(INTERP, value, strval);
-
-        return value;
-    }
-
-/*
-
 =item C<INTVAL shift_integer()>

However, pmichaud noted that Rakudo had inadvertently relied on the bug and that, though he could easily correct the problem in Rakudo, this should have been covered by a deprecation policy.

So I am reverting that commit, changing the die at line 74 of lib/Parrot/Pmc2c/PMC.pm to a warn, and posting in DEPRECATED.pod.

The next supported release is Oct 19 2010 and we can rip out the duplicated functions immediately thereafter.

Attachments

tt1785_final.diff Download (1.1 KB) - added by jkeenan 4 years ago.
Change 'warn' to 'die' and remove item from DEPRECATION.pod

Change History

  Changed 4 years ago by jkeenan

  • owner set to jkeenan
  • status changed from new to assigned
  • milestone set to 3.0
  • summary changed from src/pmc/oplib.pmc, src/pmc/resizeablestringarr.pmc: Deprecate duplicated VTABLE functions in to src/pmc/oplib.pmc, src/pmc/resizeablestringarray.pmc: Deprecate duplicated VTABLE functions in

Corrected 'Description'.

Reversion and change from die to warn performed in r48935.

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

I can understand the reasons to want a deprecation for the error.

But in the name of sanity, what is the reason to keep the duplicated vtable functions in the PMC files? Just to hear me cry?

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

Replying to NotFound:

I can understand the reasons to want a deprecation for the error. But in the name of sanity, what is the reason to keep the duplicated vtable functions in the PMC files? Just to hear me cry?

The duplication of the vtable functions was erroneous, but, as discussed on #parrot 2010-09-11, it was also the thing that needed to be put through the deprecation cycle. I took on that responsibility; hence this ticket. When touched, the code will emit warnings. After Oct 19, I'll change the warn to a die, clear the item from DEPRECATION.pod and close the ticket.

Thank you very much.

kid51

follow-up: ↓ 7   Changed 4 years ago by jonathan

20:07 <@jnthn> As far as I understand, the detection is totally fine (in fact,
               good). However, pmichaud++ felt that dying rather than just
               warning required a deprecation cycle.
20:08 <@jnthn> (I helped pm to track down and remove the duplicated v-table
               method we had somewhere in Rakudo. It's good to detect them -
               our duplication was a straightforward mistake.)
20:09 <@jnthn> oh wth
20:09 <@jnthn> The ticket is a complete mis-understanding.
20:09 <@jnthn> The duplicates serve no useful purpose and should be tossed.
20:09 <@NotFound> Sigh... Again, the duplicated vtable functions don't get
                  compiled, they aren't put in the generated C file. Look at
http://tapir2.ro.vutbr.cz/cover/cover-results/48955/c_cover/src-pmc-oplib-pmc.html
20:09 <@jnthn> There's no way we could have depended on them.
20:10 <@jnthn> It was just a simple "die vs warn" issue.
20:10 <@jnthn> NotFound++ is right here.

So, we warn rather than die for now. But the dead code that was detected can stay removed - it was dead code. :-)

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

Replying to jkeenan:

Replying to NotFound:

I can understand the reasons to want a deprecation for the error. But in the name of sanity, what is the reason to keep the duplicated vtable functions in the PMC files? Just to hear me cry?

The duplication of the vtable functions was erroneous, but, as discussed on #parrot 2010-09-11, it was also the thing that needed to be put through the deprecation cycle. I took on that responsibility; hence this ticket. When touched, the code will emit warnings. After Oct 19, I'll change the warn to a die, clear the item from DEPRECATION.pod and close the ticket.

The duplicate vtable functions were being ignored and didn't occur in the final PMC object code. It is therefore impossible that they were part of any user visible functionality. By definition, they do not fall under the deprecation policy.

What pmichaud argued falls under the deprecation policy is the behaviour of pmc2c, changes to which had caused rakudo build failures.

The vtable changes were unfortunately caught up in the revert due to being in the same commit. They should be reapplied ASAP because they add sanity at no cost.

Thank you very much. kid51

  Changed 4 years ago by jkeenan

  • milestone changed from 3.0 to 2.10

in reply to: ↑ 4   Changed 4 years ago by pmichaud

So, we warn rather than die for now. But the dead code that was detected can stay removed - it was dead code. :-)

That's all I was pointing out -- that if pmc2c dies when it encounters a duplicate vtable then it breaks language build systems (such as Rakudo's) that were inadvertently relying on the previous behavior. In no way was I claiming that the duplicate vtable entries themselves needed to be restored to the source, or that Rakudo or anything else could be relying on them.

  Changed 4 years ago by jkeenan

Corrected reversion committed in r48985. Please review.

Thank you very much.

kid51

  Changed 4 years ago by NotFound

Warnings are no longer shown. Deprecation notice looks fine for me.

Changed 4 years ago by jkeenan

Change 'warn' to 'die' and remove item from DEPRECATION.pod

follow-up: ↓ 12   Changed 4 years ago by jkeenan

  • patch set to new

Please see attachment tt1785_final.diff. Assuming I understand the issues remaining in this ticket, these changes are the last we need to perform to complete a scheduled deprecation. I will apply the patch in 1-2 days unless someone objects.

Thank you very much.

kid51

  Changed 4 years ago by NotFound

Fine for me

in reply to: ↑ 10   Changed 4 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed
  • patch changed from new to applied

Replying to jkeenan:

I will apply the patch in 1-2 days unless someone objects.

Applied in r49628. Closing ticket.

Note: See TracTickets for help on using tickets.