Replace expr command

[GoogleCodeExporter]

The command expr is different for all different systems, Linux, *Nix, BSDm, 
WinGW etc.

The majority of the expr commands can be replaced with pure shell commands.

The patch replaces all expr except for the one used for the FLAGS_ARGC variable 
which obsolete per 1.0.3.

The development of this improvement can be followed on github, 
https://github.com/petervanderdoes/shFlags/tree/feature/replace-expr-command

Original issue reported on code.google.com by pvanderdoes@gmail.com on 30 Dec 2012 at 2:53

• Blocked on: #24

[GoogleCodeExporter]

Had to delete the previous patch as it failed the test suite.

Original comment by pvanderdoes@gmail.com on 30 Dec 2012 at 4:35

[GoogleCodeExporter]

I like this patch, but it doesn't work for older shells like the Bourne shell 
on Solaris. Let me think about how to accomplish this because I do really like 
it.

Original comment by kate.w...@forestent.com on 1 Jan 2013 at 11:06

• Changed state: Accepted

[GoogleCodeExporter]

Looking over the CL more closely, I realized that the CL as stands removes 
quite a bit of functionality. For example, the validateFloat() 
validateInteger() functions have become useless for actually doing what they 
were designed to do, namely validate that the value passed is actually a float 
or integer. They are simply cutting the representative string to pieces to see 
if it fits. The patch also does not function at all for the Bourne shell of 
Solaris.

Were the unit tests run to validate the operation of the code? They should have 
failed. If not, that's a bug in itself.

As I'm in favor of reducing the usage of expr (and thereby reducing the number 
of forks necessary), I've implemented alternate validFloat() and validInt() 
functions that use built-ins, when they work. Take a look at r156 to see what 
I've done. I've benchmarked the new built-in version of validInt() at 20x 
faster compared to the expr version.

I'll look at the other ways to reduce expr usage.

Original comment by kate.w...@forestent.com on 5 Jan 2013 at 11:56

• Changed state: Started

[GoogleCodeExporter]

The check for the integer is perfectly equal to what it was, all tests passed 
(That's how I found the bug that I fixed in issue 22).

ValidateInteger:
First there is a check if the input starts with a dash (negative sign), if so 
we strip the the first dash only.
Next we check if the input fits in an empty string or a string with a 
non-digit, if it does the check fails.

ValidateFloat:
If it's an integer the check passes,
We split the given input into two parts, Part1 before the first dot and Part2 
after the first dot
Part1 should be an integer otherwise it fails
If Part2 is an empty string or a string with a non-digit the check fails.

I didn't know it would fail for the Bourne shell, but with the check you build 
in to see if the built-in is supported that problem is solved.

Original comment by pvanderdoes@gmail.com on 5 Jan 2013 at 2:10

[GoogleCodeExporter]

Wow, I'm not sure what patch I looked at. I had something else in mind 
completely, but I'm mistaken. I liked your case statement even better than my 
built-in method as it should be compatible with Bourne. I'll rework again.

Original comment by kate.w...@forestent.com on 6 Jan 2013 at 4:33

[GoogleCodeExporter]

New patch rebased on r171.

Original comment by pvanderdoes@gmail.com on 10 Jan 2013 at 7:32

Attachments:

• 0001-Faster-builtin-functions.diff

[GoogleCodeExporter]

I've reworked things again, incorporating some of the best bits of your most 
recent patch. The only usages of 'expr' now are for those OSes that do not 
support the shell built-ins.

Original comment by kate.w...@forestent.com on 15 Jan 2013 at 12:15

[GoogleCodeExporter]

Original comment by kate.w...@forestent.com on 15 Jan 2013 at 12:19

• Now blocked on: #24

[GoogleCodeExporter]

Fixed in r190.

Original comment by kate.w...@forestent.com on 17 Jan 2013 at 7:40

• Changed state: Fixed