Forum OpenACS Development: empty_string_p obsolete?

Collapse
Posted by Daniël Mantione on
Since TCL now has an eq operator (boolean string equal) for a while, there should be no need for empty_string_p anymore.

I.e.:

if {$string eq ""} then {}

is fully equivalent to

if {[empty_string_p $string]} then {}

If I'm editing OpenACS code, I'm find myself replacing empty_string_p with the eq operator quite often, as it is faster and better readable.

What do the other people here think of this?

Collapse
Posted by Malte Sussdorff on
If it is faster than why not? Good you brought this to our attention.
Collapse
Posted by Don Baccus on
For quite some time now we've had [string equal $string ""], and I've routinely rewritten empty_string_p code to use this as I've been working on packages.

The even newer "eq" (which came in 8.4, I think???) is even more welcome, more readable, easier to type, and more like a real computer language :)

So - go for it! Make those changes, sure.

Collapse
Posted by Gustaf Neumann on
Tcl 8.4 was released 2002. If you are updating code, please note that there is a 'ne' operator as well. Comparisons like '$somevar == "someflag"', where no numerical comparison is wanted, should use 'eq' and 'ne' as well. The operators '==' and '!=' should be used for numerical comparisons only.

As we are on the matter: The argument of expr should be practially always written between curley brackets, otherwise expr is not byte-code-compiled. Code like 'set a [expr 1+$b]' should be written as 'set a [expr {1+$b}]'.

Collapse
Posted by Malte Sussdorff on
Could we do a regexp and replace all "[empty_string_p" in the sourcecode with "[string eq """ ? After that we could still argue to run a second round of replacement which just uses the eq an ne parameters?
Collapse
Posted by Dave Bauer on
Yes we should do that. $var eq "" or $var ne "" is fastest and OpenACS 5.2 requires Tcl 8.4 so there is no compatbility issues. Let's get this done soon on HEAD so we can have it fully tested for 5.3/
Collapse
Posted by Gustaf Neumann on
well, replacing the occurrences is not completely trivial since the substitution can not be made blindly and requires some context knowledge. If there is e.g. "if [empty_string_p $string] {...}" then the substitution to "if $string eq "" {...}" is invalid. Furthermore, if the old code is "func [empty_string_p $string]", we have the same problem. It is worth while to replace the [string equal $a b] as well. Here it is e.g. necessary to quote literal b, otherwise the resulting code is invalid. There are a few more nasty cases...

i wrote a little program for this update that alters the files and makes backup files. if i run it over my installation i get 8197 changes in 1590 files. oacs+dotlrn start up after this change, and some random clicking around seems to work. However it is highly probable, that the change will break some code. It would be nice to have a regression test suite for such cases which succeeds on all tests in cvs, then this change could be done quite fast.... i will do some more testing and make then the program available. If there is interest, i can alter the cvs head version.

Collapse
Posted by Gustaf Neumann on
Hi everybody,

I did some more work on these issues and developed and tested the tcl script below that changes and normalizes various expressions. The script normalizes the following expressions (somewhat simplified and not complete)

  • ![empty_string_p $X] ... {$X ne ""}
  • [empty_string_p $X] ... {$X eq ""}
  • ![string equal $X $Y] ... {$X ne $Y}
  • [string equal $X $Y] ... {$X eq $Y}
  • ![string compare $X $Y] ... {$X eq $Y}
  • [string compare $X $Y] ... {$X ne $Y}
  • ![string match "" $X] ... {$X ne ""}
  • [string match "" $X] ... {$X eq ""}
  • [string match "stringwitoutmatchchars" $X] ... {$X eq "stringwithoutmatchchars"}
  • $X == "alpha" ... {$X eq "alpha"}
  • $X != "alpha" ... {$X ne "alpha"}
Furthermore, the adds braces around (simple) expressions such they will get compiled into byte code. Since eq and ne are well optimzed by the bytecode compiler, there should be a measurable performance improvement for many functions.

I tried to be rather conservative, so it does not changes all ocurrences of these patterns, but only rather simple cases. Complex expression should be changed by hand.

On my machine, the script produces 11279 changes in 1858 files. This is most probably the biggest change (in number of lines) in the history of oacs. By doing some random clicking in my oacs 5.2 + dotlrn installation, everything seems to work. But since the number of changes is huge, i would be surprised, if there are no bugs somewhere hidden. I did not commit the changes into cvs but decided to post the script here such that package maintainer and OCT members might consider these changes already into e.g. 5.2.

If the script is run e.g. from the ..../packages directory,

tclsh /SOMEPATH/normalize_expressions.tcl
it processes all *tcl-files underneath it and produces a backup file for each changed file with the name $name-original. One can switch back to the original state by running
tclsh /SOMEPATH/normalize_expressions.tcl -reset 1 -change 0
With these flags it will simply overwrite the source files with the *original files. When the changes are committed, the *-original files should be deleted.

Here is the script: http://media.wu-wien.ac.at/download/normalize_expressions.tcl

my best seasons greetings
-gustaf

PS: I found some strange looking [string match ...] occurrences (not changed by the script) that someone should give a look. Some of these might be string comparisons (not matches), some seem to have the wild card characters in the wrong argument. Here are a few examples:

gatekeeper/tcl/gatekeeper-procs.tcl: if {[string match $port ""]} {
acs-tcl/tcl/tcl-documentation-procs.tcl: [string match $lcase_value "f"] || \
news/www/preview.tcl:if { [string match $action "News Item"] } {
acs-bootstrap-installer/db-init-checks-oracle.tcl: if {($dbversion_total <= 8001006) && [string match $platform "Linuxi386*"]} {
acs-bootstrap-installer/tcl/00-proc-procs.tcl: if { $impl eq "impl" || [string match $impl "impl::*"] } {

Collapse
Posted by Malte Sussdorff on
Excellent work Gustaf. I think we could savely apply this to HEAD. As for 5.2, I'd not do it before 5.2.1 (read: before we tagged 5.2 in a way that it shows up in the repository and OpenACS has been upgraded).
Collapse
Posted by Gustaf Neumann on
Just detected a bug in the generated code (a case, where there was no curly bracket around the expr in the original code). please re-fetch the script if necessary and run "normalize_expressions.tcl -reset 1" to redo the changes.
Collapse
Posted by Gustaf Neumann on
i have updated normalize_expressions.tcl on the link above, to cover some more cases....
Collapse
Posted by Malte Sussdorff on
I think unless someone else does it I will do it middle/end of January on HEAD an on the 5.2.1 branch if 5.2 has been properly released. Does the commit on 5.2.1 need a TIP?