Re: [RFC] A patch for parse_number (c-exp.y) to recognize 1.25f

[Daniel Jacobowitz <drow@false.org>]

On Thu, Sep 15, 2005 at 04:05:48PM +0800, Wu Zhou wrote:
> 
> > To fix this kind of false-negative and not to introduce false-positive, I 
> > am thinking of changing the second format in sscanf to "%s" (string).  
> > Thus the parser could get the trailing characters following the float. 
> > Then we can add some code to parse them to eliminate false-positive.  Any 
> > points on this idea?  Is it worthwhile to do this?  Please comment.  TIA.
> 
> Following the above point, I coded another patch.  It can fix the problem 
> and don't introduce any false-positive mentioned above.  I had also coded 
> a testcase for that.  Had tested on i386.  With the patched GDB, it 
> reported 18 PASS; with the original GDB, it report 9 PASS and 9 FAIL (all 
> of these failure is due to the fact that GDB can't handle the suffix 
> following the float constants.)
> 
> IMHO, there are yet another advantage for this patch: it is easy to be
> extended to handle other suffixes, such "df" for _Decimal32, "dd" for 
> _Decimal64 and "dl" for _Decimal128, all of which are described in IEEE 
> 754R for decimal floating point.
> 
> Here goes the patch and the testcase.

This looks good.  I have some comments on formatting and testing, but
that's it.

> 2005-09-15  Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* c-exp.y (parse-number): Modify the float parsing logic to let it 
> 	recognize the suffix.

"a suffix" rather than "the suffix", please.

> !       if (num == 1)
> ! 	putithere->typed_val_float.type = builtin_type (current_gdbarch)
> ! 					    ->builtin_double;

Instead of splitting this line (and the two below) in the middle of a
single reference, you can split it at the assignment.  Like this - two
spaces in the continued line:

	putithere->typed_val_float.type
	  = builtin_type (current_gdbarch)->builtin_double;

Emacs'll butcher your version, I'm afraid.

> Index: gdb.base/bfp-test.c
> ===================================================================
> RCS file: gdb.base/bfp-test.c
> diff -N gdb.base/bfp-test.c
> *** /dev/null	1 Jan 1970 00:00:00 -0000
> --- gdb.base/bfp-test.c	15 Sep 2005 06:49:51 -0000
> ***************
> *** 0 ****
> --- 1,15 ----

It's a pretty trivial test case; but please add a copyright notice
anyway.

> + # Run to the breakpoint at return.
> + set bp_location [gdb_get_line_number "return"]
> + gdb_test "break $bp_location" \
> +     "Breakpoint.*at.* file .*$srcfile, line $bp_location\\." \
> +     "breakpoint at return"
> + gdb_test "continue" \
> +     "Continuing\\..*Breakpoint.*" \
> +     "continue to breakpoint"

This is fine, but you can also use gdb_breakpoint and
gdb_continue_to_brekpoint or gdb_continue.

> + # Test that gdb could handle the above correctly with "set var" command.
> + send_gdb "set var b32=10.5f\n"
> + gdb_expect {
> +     -re "$gdb_prompt $" { pass "set var b32=10.5f" }
> +     -re "Invalid number" { fail "do not recognize 10.5f" }
> +     timeout     {fail "set var b32=10.5f" }
> +   }

Please don't use "send_gdb" and "gdb_expect" directly.  Also, the
passing and failing tests should have the same message - an optional
string at the end in parentheses explaining the failure is OK, but
anything else is bad for automated testing.

gdb_test_multiple should work OK everywhere you used send_gdb.

> + # Test that gdb could handle invalid suffix correctly.
> + send_gdb "set var b32=100.5a\n"
> + gdb_expect {
> +     -re "$gdb_prompt $" { fail "don't report error on 100.5a" }
> +     -re "Invalid number" { pass "100.5a is invalid number" }
> +     timeout     {fail "set var b32=100.5a" }
> +   }

... although all of the invalid tests could just use gdb_test.

-- 
Daniel Jacobowitz
CodeSourcery, LLC