Re: [TCLCORE] [string insert] again

[Dipl. I. S. G. B. <se...@us...>]

 

Hi, 

I took a look resp. made a small code review [1] of the branch
amg-string-insert [2].

Thank you for useful feature!

Imho, looks good, but...

1. For several reasons, I would like to see that without a new
instruction INST_STR_INSERT, because it is 
very-very similar to INST_STR_REPLACE and TclCompileStringInsertCmd can
be relative easy rewritten 
using INST_STR_REPLACE (instead of new instruction) with small
enhancement in corresponding 
execution part in TEBCresume for case INST_STR_REPLACE; 

2. Many code-pieces, that could be rewritten more tclish (better
readable or just shorter) like: 

- for {set noComp 0} {$noComp <= 1} {incr noComp} {
+ foreach noComp {0 1} {

BTW. Instead of "duplicating" of the test-cases in "string.test" (with
for cycle), you can just create a 

new script e. g. "string-not-compiled.test", like:

# string-not-compiled.test

if {[lsearch [namespace children] ::tcltest] == -1} {
 package require tcltest
 namespace import -force ::tcltest::*
}

# rewrite with non-compiled eval:

interp alias {} run {} testevalex

set constraints testevalex

rename tcltest::Eval tcltest::CompiledEval

proc tcltest::Eval {script args} {

 set script [list run $script]
 uplevel 1 [list tcltest::CompiledEval $script {*}$args]

}

# call test:

source string.test

Thus it can be tested separately, e. g. not compiled only: 

tclsh ../tests/all.tcl -file "string-not-compiled.test" 

or together e. g.: 

tclsh ../tests/all.tcl -file "string*.test" 

But IMHO the package tcltest can be extended to provide common mechanism
to test compiled as well as not compiled (without extra programming
overhead for rewriting the test-cases).
Just as switch resp. option...

3. My performance test cases show a little degradation everywhere
(excepting "string replace"), but it can be a consequence of the
introducing of new instruction... I'll try to find the reason later (ATM
very complex to profile resp. to compare the profiler output with trunk,
I should rewrite my analyses scripts, they are currently for 8.6-based
branches). 

4. I didn't understand what happens with test-cases from
stringComp.test. Totally removed? rewritten? Obsolete?

I've currently very few time make a full review, but I'll try to find
the time next week... 

Regards,
sebres. 

Am 05.10.2017 17:11, schrieb Andy Goth: 

> Any progress on [string insert]? Has anyone had a chance to review the code or read the TIP in detail? I don't want this to hang out forever, I'd like a chance to resolve any issues anyone may have then eventually have this be voted on. Like I said at the start of the project, I want to get a comparatively simple project completed before I resume work on the array enumeration C API project which is quite a bit more complex.
 

Links:
------
[1]
http://core.tcl.tk/tcl/vdiff?from=6954ead823bc8378&amp;to=e2718c364b48bc1b&amp;sbs=1
[2]
http://core.tcl.tk/tcl/timeline?r=amg-string-insert&amp;nd&amp;c=2017-09-23+16%3A36%3A49