Ticket #728 (closed bug: fixed)

Opened 5 years ago

Last modified 5 years ago

r39273 breaks 'make test' for tcl

Reported by: coke Owned by: bacek
Priority: normal Milestone: 1.3
Component: none Version: trunk
Severity: medium Keywords: tcl blocker
Cc: Language:
Patch status: Platform:

Description

As of r39273, partcl's 'prove t/tcl_misc.t' fails some tests.

the transformations in src/grammar/expr/past2pir.tg begins to generate invalid code.

Change History

  Changed 5 years ago by coke

  • keywords tcl blocker added
  • milestone set to 1.3

  Changed 5 years ago by bacek

  • owner set to bacek

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

Replying to coke:

As of r39273, partcl's 'prove t/tcl_misc.t' fails some tests. the transformations in src/grammar/expr/past2pir.tg begins to generate invalid code.

Ok... There is two versions which fix this ticket:

diff --git a/src/pmc.c b/src/pmc.c
index 506aa05..a81559a 100644
--- a/src/pmc.c
+++ b/src/pmc.c
@@ -258,9 +258,13 @@ Parrot_pmc_try_reuse(PARROT_INTERP, ARGIN(PMC *self), ARGIN
     if (dest->vtable->flags & VTABLE_IS_READONLY_FLAG)
         return pmc_new(interp, VTABLE_type(interp, self));
 
-    /* It's safe in this case. Caller want to replace us anyway */
+    /*
+       It's NOT safe in this case.
+       Caller want to replace us but calls to C<self> and C<dest> can be mixed.
+       See TT#728 for reference.
+    */
     if (dest == self)
-        return dest;
+        return pmc_new(interp, VTABLE_type(interp, self));
 
     /* Morph to self type is required */
     {

and

bacek@icering:~/src/parrot$ git diff src/pmc/
diff --git a/src/pmc/scalar.pmc b/src/pmc/scalar.pmc
index 70b591a..600b9f5 100644
--- a/src/pmc/scalar.pmc
+++ b/src/pmc/scalar.pmc
@@ -1069,7 +1069,7 @@ Concatenate the string C<value> in place.
         STRING * const s = Parrot_str_concat(INTERP,
             SELF.get_string(), value, 0);
 
-        dest = Parrot_pmc_try_reuse(INTERP, SELF, NULL, dest);
+        dest = pmc_new(INTERP, VTABLE_type(INTERP, SELF));
 
         VTABLE_set_string_native(INTERP, dest, s);
         return dest;

I can understand why former is better, but epically fail to understand why latter fix problem too...

  Changed 5 years ago by coke

While we try to figure out the best way to do what the original revision intended, backed it out in r39309

  Changed 5 years ago by bacek

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

Idea of reusing pmcs was discarded, related commits reverted. Resolving ticket.

Note: See TracTickets for help on using tickets.