Tcl Source Code

The documentation for [upvar] says:

  upvar ?level? otherVar myVar ?otherVar myVar ...?

which makes it pretty clear that the presence of a level is determined by the parity of the number of args: odd means level, even means no level.

However, "upvar 1 x" raises a "wrong # args" error !

This is due to the fact that the implementation ascribes higher priority to the value of the first argument ("integer affinity") than to positional criteria.

This is Bad Thing (1) as it doesn't work as documented and (2) as it induces more verbose style, nullifying the intention of the "default level 1": programmers now routinely write "upvar 1 $x y" instead of the equivalent "upvar $x y" in fear of the above, insane behavior for an incoming integer-named variable.

So I post this report proposing, as a fix, to just do as documented. That is, "upvar 1 x" should have the same effect as "upvar 1 1 x".

Prio 8 as "low-hanging fruit" since (1) the patch is trivial and (2) the fix turns error cases into valid behavior in a way that really cannot harm a sane program.

Patch to be shortly uploaded.

allow_comments - 1

Looked good to me. Applied (after tweaking variable names).

File Deleted - 318435:

Done.

Please delete the patches that you don't want me to review :)

File Added - 318446: upvar4.patch

typo in previous message, should read "... since _uplevel_ has no constraints..."

Attaching patch including extra test for [upvar 1 x], and style (hopefully) conformingto the Eng.Man. 
File Added: upvar4.patch

"Folks should just code the level explicitly" is just contrary to 10+ years of unchanged documentation and usage, and also to the "Huffmann principle", meaning that the dominant use should be written concisely.

The name value "1" in TOGF *is* used on the error path, to generate the message 'bad level "1"' when the requested implicit level 1 is nonexistent (like 'upvar x y' at toplevel).

About the generalization to [uplevel], it is impossible since upvar has no constraints on the parity or #args, so positional/parity cues are not usable.

well, I reviewed upvar2.patch.

If upvar3.patch is significantly
different, ignore everything I said.

I'm done with this one.

On a superficial level, the code doesn't follow
the usual Tcl Engineering Manual conventions, and
would be better reformatted before commit.

On substance, the decision to accept comes down
to a judgment on whether to object to replacing
one error with another, as demonstrated in the
changes to test upvar-8.3.  I'm the wrong one
to make that judgment.  At the moment I just don't
care.  I suspect when I'm in a better mood, I
still won't care.  Folks should just code the
level explicitly.

As I understand it, the identifiable case that
is most clearly an actual bug to fix is

    upvar 1 x

so it would be good to have a test case
for that, and check that the docs are clear
that the interpretation choice on it is
correct.

In the niggly details, I don't see the value
of setting name to "1" in TclObjGetFrame().
The value isn't used.

I'm left wondering about the impact on
other commands that have level values which
are optional, such as [uplevel].  Not curious
enough to go looking though.  If there's a
general problem, it's best to commit all
solutions at once.

File Added - 318435: upvar3.patch

Updating patch to HEAD
File Added: upvar3.patch

Reassigning to dgp for review.

File Added - 317096: upvar2.patch

Updated patch modifies slightly TOGF, but in a compatible manner, by using NULL instead of the empty string as a magic value to get level 1. Suggestion by dkf. Also, improves the error message in case of toplevel access:

     upvar x y ;# at toplevel
     bad level "1"
File Added: upvar2.patch

As a side note, the TDK static syntax checker does check the command using odd/even to determine the presence of a level argument.

File Added - 316886: upvar.patch

Attaching the promised patch.

Please note that this patch insists on touching only Tcl_UpvarObjCmd, and not TclObjGetFrame, despite the latter having a very contorted domain (it maps many invalid strings to level 1 silently, and barfs only on out-of-range levels). The reason is that (1) TOGF is stubbed and hence its semantics should not be changed lightly, and (2) it is still reasonably efficient to check this from outside.

Also note that the compiled-upvar paths don't need changing because (1) when the first arg is not known at compile time, upvar switches to runtime; and (2) when the first arg is an integer *and* the total number of args is even, the same switch occurs (initially thinking it would raise an error; but now it will Just Work).
File Added: upvar.patch