search fails to fully unset the data in the data array for each loop about speedtables HOT 30 CLOSED

My first thought was "yes, this should probably be fixed, but it's not critical since the documentation doesn't make any promises either way". But there might be an issue with nulls not getting cleared ( -array vs -array_with_nulls ) which would definitely be a bug. The related pgtcl bug may have the same problem as well.

from speedtables.

No, in speedtables at least it does explicitly clear the array element on array_set.

from speedtables.

I've never used -array_set, only -array.

from speedtables.

You mean array_with_nulls? array_set is an internal routine ($table_array_set) that implements array. It takes care of clearing nulls.

There doesn't seem to be a straight Tcl C API call to clear an array, so it's not going to be a one-line fix.

This is unrelated to stapi.

from speedtables.

Oh, nevermind, I see what you're saying now. Thanks.

from speedtables.

Looking at the code in ctable_search.c I'm not seeing anything changed that would fix this. case CTABLE_SEARCH_ACTION_ARRAY is not clearing the array. You sure you can't reproduce?

from speedtables.

I can still reproduce. My earlier statement to the contrary was in error.

from speedtables.

Tcl_UnsetVar2 is the public API. We can call that before setting it, but should it check if there is already a non-array var with that name and error out? For consistency, using a non-array variable name here should be an error.

Or am I overthinking it? The semantics of unset and array unset in Tcl are a little mixed up. Should ctables even care?

unset a - unsets a whether it's a variable or an array, errors out if a doesn't exist
array unset a - only unsets a if it's an array, errors out if a is a var, no result if a doesn't exist

from speedtables.

"search" already errors if you supply a name that is already used by a non-array variable, but not until it has found a row and actually tries to set the first element of the array, so there is already an undocumented contract that the name must not already exist, or it must be an array.

So doing the "array unset a" proposal would be the least impact on breaking existing code.

from speedtables.

Yes, that's the behavior I would expect, and that's the behavior that would be correct. It's implicit in the semantics of Tcl arrays.

There's no published C API for that, though, and it looks like the code in generic/tclVar.c:4191 (Tcl 8.6 source) uses internal APIs. So we'd have to clear the array at the end of the loop (see attached diff below).

diff.txt

The current semantics leave the last array value intact after the table-search is complete, and there may be code depending on that. This would break that, and surprising as it may seem I wouldn't rule out someone having written code that uses it.

from speedtables.

I might propose unsetting the array before each row, and then finally prior to returning after the last row. That would ensure no interlopers present initially in the array.

from speedtables.

I'm actually concerned about unsetting it at the end of the row.

I would rather do effectively an "array unset" at the beginning, but I don't see how to do that with just the published Tcl APIs, unless I just stomp on any existing variable with the name. That would be easier: just put

Tcl_UnsetVar2(interp, Tcl_GetString(search->rowVarNameObj), NULL, 0);

at the beginning of the case CTABLE_SEARCH_ACTION_ARRAY_WITH_NULLS/CTABLE_SEARCH_ACTION_ARRAY.

from speedtables.

Something like this.

from speedtables.

Sure, leaving the last row set might be safer in case anyone was using "break" and expecting it to be left in existence.

Your last example seems reasonable.

from speedtables.

Tcl_UnsetVar2 is the way to go. If you want to check the first time to see if there is a variable there, which seems to me to be fairly good, then do a Tcl_GetVar2Ex(NULL, arrayName, NULL, 0) and if it returns non-null then return an error.

Send NULL for interp instead of interp else Tcl will put an error message in the result object and set the errorCode for the normal case when the var doesn't exist.

Hmm it may be harder than this if there is already an array present instead of a var.

If there is an array present I agree with @bovine 's point of ensuring no interlopers present initially in the array.

from speedtables.

That's clever, Karl. Roger wilco.
On Apr 27, 2016 07:37, "Karl Lehenbauer" [email protected] wrote:

Tcl_UnsetVar2 is the way to go. If you really want to check the first time
to see if there is a variable there then do a Tcl_GetVar2Ex(interp,
arrayName, NULL, 0) and if it returns non-null return TCL_ERROR and since
you passed interp instead of null the result object will already have
"variable isn't an array" and errorCode will be set to TCL LOOKUP VARNAME x.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#56 (comment)

from speedtables.

[previous comment based on email from Karl's comment, not edited version]

If you don't pass interp, then how does it know the context to look up the variable name?

The snippet I provided should work without clobbering anything in interp, at the cost of calling Tcl internal routines. Or it can clear the error state of the interp.

Definitely agree this should only be done first time through. Actually, it could do it while parsing the arguments.

from speedtables.

Busted. You're right. I was thinking about some of the other object-manipulating functions. As long as you don't "or" TCL_LEAVE_ERR_MSG with the flags then you'll be all right on the error message.

from speedtables.

Doesn't work, Tcl_GetVar* on an array returns the array... which stringifies to a key-value list:

Tcl_ObjGetVar2 has value 'id brock name {Brock Sampson} home {Venture Compound} show {Venture Bros} alive 1 gender male age 35 coolness 100 _key brock' as string
foo is not an array

(I used Tcl_ObjGetVar2 to avoid an unnecessary Tcl_GetString() but that shouldn't make a difference)

from speedtables.

I can try setting a dummy element in the array with Tcl_SetVar2Ex.

from speedtables.

That seems to work. It's a bit of a kludge, though.

patch2.txt

from speedtables.

Yeah that's gross. How about if we just document it as unsetting anything if it's already there and move on with our lives.

from speedtables.

The original behavior is also not technically wrong, it's just surprising. We could simply document that.

Pgtcl has the same behavior ( see flightaware/Pgtcl#4 ).

from speedtables.

The original behavior was found to have caused an obscure bug that was affecting multicom greatly. We want to prevent similar problems from unintentionally occurring in the future.

from speedtables.

OK, I'll do it.

I take it that you're nixing using tcl internals @lehenbauer ?

from speedtables.

Yeah just nuke the array/var at the start of the first pass and the end of each pass, I think.

from speedtables.

We discussed that it would be better to only nuke it at the start of each pass, and not at the end. That would minimize impact on code that assumed the last row would be retained if you break out.

from speedtables.

Committed to branch "issue56" with tests updated. Merge when satisfied.

from speedtables.

Merged.

from speedtables.

Branch deleted.

from speedtables.