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

  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(-)
  

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 ']')?
  

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?
  

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.
  

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.
  

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.
  

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.
  

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.
  

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