[PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Martin Galvan]

Seems like an obvious fix to me, but I wanted to check just in case.
Ok to commit?

---
 gdb/testsuite/ChangeLog   | 4 ++++
 gdb/testsuite/lib/gdb.exp | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index e01f883..95338b3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2015-06-13  Martin Galvan  <martin.galvan@tallertechnologies.com>
+
+	* lib/gdb.exp (test_class_help): Escape a loose '['.
+
 2015-06-12  Antoine Tremblay  <antoine.tremblay@ericsson.com>
 
 	PR breakpoints/16465
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 41797e7..2be9ba4 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -4674,7 +4674,7 @@ proc test_class_help { command_class expected_initial_lines args } {
     set l_stock_body {
         "List of commands\:.*\[\r\n\]+"
         "Type \"help\" followed by command name for full documentation\.\[\r\n\]+"
-        "Type \"apropos word\" to search for commands related to \"word\"\.[\r\n\]+"
+        "Type \"apropos word\" to search for commands related to \"word\"\.\[\r\n\]+"
         "Command name abbreviations are allowed if unambiguous\." 
     }
     set l_entire_body [concat $expected_initial_lines $l_stock_body]
-- 
2.4.3

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Yao Qi]

Martin Galvan <martin.galvan@tallertechnologies.com> writes:

> Seems like an obvious fix to me, but I wanted to check just in case.
> Ok to commit?

Looks the code works well without this patch.  Do we really need to
escape '[' (and ']')?

-- 
Yao (齐尧)

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Doug Evans]

On Mon, Jun 22, 2015 at 4:10 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Martin Galvan <martin.galvan@tallertechnologies.com> writes:
>
>> Seems like an obvious fix to me, but I wanted to check just in case.
>> Ok to commit?
>
> Looks the code works well without this patch.  Do we really need to
> escape '[' (and ']')?

In that case there's still something wrong, or at least still
something requiring patching.

When code isn't consistent readers start wondering if there's a problem,
and may decide to look into it.
And if there isn't a problem then it's just wasted time.

Here the inconsistency happens because it turns out that
in this context [ and \[ have the same effect.
[I added some printfs to gdb_test to find this out.]

I think that's a bug but I don't know what would be involved
in fixing it. OTOH, we should at least make the code consistent.

Generally [ is escaped to avoid Tcl treating it as a subexpression to evaluate.
But the escaping isn't needed if the string is wrapped in {} braces
which is the case here. Thus to me it feels like it's all the other
escaped brackets that need to be fixed (by changing \[ \] to [ ]).

OTOH, if one carried that through to completion it would
involve a *lot* of changes. help.exp is replete with them.
So for now I think the thing to do is apply this patch,
*and* add a comment somewhere (the function comment
for gdb_test?) documenting that [ == \[.

Thoughts?

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Martin Galvan]

On Tue, Jun 23, 2015 at 12:50 PM, Doug Evans <dje@google.com> wrote:
> So for now I think the thing to do is apply this patch,
> *and* add a comment somewhere (the function comment
> for gdb_test?) documenting that [ == \[.
>
> Thoughts?

If Yao thinks it's ok then I can send a v2 including that comment.

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Doug Evans]

On Tue, Jun 23, 2015 at 10:50 AM, Doug Evans <dje@google.com> wrote:
> Here the inconsistency happens because it turns out that
> in this context [ and \[ have the same effect.
> [I added some printfs to gdb_test to find this out.]
>
> I think that's a bug but I don't know what would be involved
> in fixing it.

Actually, this is arguably s.o.p. Bleah.
Otherwise \r\n inside {} won't get expanded as CR,LF.
E.g. compare
set foo {a\r\nb}
vs
set foo "a\r\nb"

That \[ gets expanded as [ is no different than \z getting expanded as z.
% set foo "a\zb\[c"
azb[c
%

Still a bit confusing since \[ vs [ different in most contexts in Tcl.
So I'd still advocate for a comment.

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Yao Qi]

Doug Evans <dje@google.com> writes:

> Generally [ is escaped to avoid Tcl treating it as a subexpression to evaluate.
> But the escaping isn't needed if the string is wrapped in {} braces
> which is the case here. Thus to me it feels like it's all the other
> escaped brackets that need to be fixed (by changing \[ \] to [ ]).
>
> OTOH, if one carried that through to completion it would
> involve a *lot* of changes. help.exp is replete with them.
> So for now I think the thing to do is apply this patch,
> *and* add a comment somewhere (the function comment
> for gdb_test?) documenting that [ == \[.

[ and \[ may be used everywhere in the testsuite, so I don't know how
much useful that we add comment that [ == \[ for gdb_test.  Nobody will
realise such difference is documented in the gdb_test comment.

I agree that it is impractical to change every \[ to [, but in this
patch, I am inclined to do the change in the reversed direction, which
is to change \[ to [ within proc test_class_help.

-- 
Yao (齐尧)

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Doug Evans]

On Tue, Jun 23, 2015 at 11:28 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Doug Evans <dje@google.com> writes:
>
>> Generally [ is escaped to avoid Tcl treating it as a subexpression to evaluate.
>> But the escaping isn't needed if the string is wrapped in {} braces
>> which is the case here. Thus to me it feels like it's all the other
>> escaped brackets that need to be fixed (by changing \[ \] to [ ]).
>>
>> OTOH, if one carried that through to completion it would
>> involve a *lot* of changes. help.exp is replete with them.
>> So for now I think the thing to do is apply this patch,
>> *and* add a comment somewhere (the function comment
>> for gdb_test?) documenting that [ == \[.
>
> [ and \[ may be used everywhere in the testsuite, so I don't know how
> much useful that we add comment that [ == \[ for gdb_test.  Nobody will
> realise such difference is documented in the gdb_test comment.

That's not a reason to not document it at all.
If I can't something to the code I'll add a note to the wiki.

> I agree that it is impractical to change every \[ to [, but in this
> patch, I am inclined to do the change in the reversed direction, which
> is to change \[ to [ within proc test_class_help.

Fine by me.

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Andreas Schwab]

Doug Evans <dje@google.com> writes:

> Actually, this is arguably s.o.p. Bleah.
> Otherwise \r\n inside {} won't get expanded as CR,LF.

\r\n are also special for tcl regexps.  So it doesn't matter whether
they are expanded by the tcl parser or the regexp engine.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PING][PATCH] testsuite: Escape a loose '[' character inside a regexp.

[Doug Evans]

On Tue, Jun 23, 2015 at 12:04 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Doug Evans <dje@google.com> writes:
>
>> Actually, this is arguably s.o.p. Bleah.
>> Otherwise \r\n inside {} won't get expanded as CR,LF.
>
> \r\n are also special for tcl regexps.  So it doesn't matter whether
> they are expanded by the tcl parser or the regexp engine.

Sorry, yeah.
http://tcl.tk/man/tcl8.5/TclCmd/re_syntax.htm