Suspicious code in sub.c - Parrot_capture_lex - Multi

[Util]

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.

[pmichaud]

Now fixed in r37100, thanks!

Pm