Ticket #398 (closed bug: fixed)

Opened 6 years ago

Last modified 6 years ago

Suspicious code in sub.c - Parrot_capture_lex - Multi

Reported by: Util Owned by:
Priority: normal Milestone:
Component: core Version: trunk
Severity: medium Keywords:
Cc: pmichaud, cotto, Util Language:
Patch status: Platform:

Description

In r33193, Parrot_capture_lex was added to src/sub.c, as part of merging the lexicals branch into trunk. In the section commented as "MultiSub gets special treatment", this code originated:

if (!PMC_IS_NULL(child_sub->outer_sub)) 
    if (0 == string_equal(interp, current_sub->subid, 
                          PMC_sub(child_sub->outer_sub)->subid)) { 
    old = child_sub->outer_ctx; 
    child_sub->outer_ctx = Parrot_context_ref(interp, ctx); 
    if (old) 
        Parrot_free_context(interp, old, 1); 
} 

The code has three formatting problems. The outer if lacks braces, the closing brace of the middle if is lined up with the outer if, and the code inside the middle if is not indented. These problems make it too easy to cause inadvertent semantic changes during future modifications, especially by inserting code between the outer and middle ifs.

In r37016, I think such a inadvertent semantic change occurred during the change to ATTR accessors.

Either
1. the original code was correct, and now a new codepath is opened when PMC_IS_NULL(child_sub->outer_sub), or
2. the original code was incorrect in blocking the PMC_IS_NULL(child_sub->outer_sub) codepath, and the problem is now corrected, or
3. my analysis is flawed.

If (2), then the current code needs to change back to the prior codepath, by enclosing the outer if in braces.

If (1) or (2), then Parrot_capture_lex is under-exercised by the test suite. No test detected the change in behavior.

In any case, the code needs to be re-indented.

Other eyes than mine are needed to address this issue; I do not grasp the meaning of the code.

Change History

Changed 6 years ago by pmichaud

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

Now fixed in r37100, thanks!

Pm

Note: See TracTickets for help on using tickets.