Re: [PATCH] [gdb/testsuite] test a function call by hand fron pretty printer

[Joel Brobecker <brobecker@adacore.com>]

> > > +# This file is part of the GDB testsuite.  It tests Python-based
> > > +# pretty-printing for the CLI.
> > > +
> > > +load_lib gdb-python.exp
> > > +
> > > +standard_testfile
> > > +
> > > +# Start with a fresh gdb.
> > > +gdb_exit
> > > +gdb_start
> > 
> > I think you can drop the gdb_exit/gdb_start part (it is taken care
> > of by prepare_for_testing).
> 
> If I remove this, skip_python_tests throws a TCL error, complaining
> about not having a use_gdb_stub variable. If I change the order of the
> 2 if statements, the error isn't there anymore but we compile the code
> and don't use it. Do I keep the exit+start here and document why I
> need it, or do I change the order of the if statements?

Grumpf. This is a bug in skip_python_tests_prompt, in my opinion,
which seems to make an unwarranted assumption about a GDB being
available. It looks like a simple call to "gdb_start" at the start
of that function should be sufficient to fix the issue, as gdb_start
only starts GDB if one hasn't already been started.

But I don't want this fix to be a precondition to your patch.
In the meantime, I think a comment explaining why is good enough.
I keep flip-flopping between the two options and can't decide,
so I went with the simplest and less wasteful (avoiding to do
build a test program if the testcase is not going to run).

> > There is an oddity here; the name of the function seems to indicate
> > that this should be "off" rather than "on", here. Can you confirm
> > whether this is correct or not?
> 
> It is, the name of the function is wrong. More details below
> 
> > 
> > > +    set remote_python_file [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> > > +    gdb_test_no_output "source ${remote_python_file}" "load python file"
> > > +
> > > +    return 0
> > > +}
> > > +
> > > +proc start_test_pretty {} {
> > 
> > This function seems to be identical to the start_test_no_pretty.
> > Can you perhaps factorize this into a single function, possibly
> > with one argument being the "on/off" value to be used for
> > the "print pretty" setting?
> > 
> > If, for some reason I missed, it is better not to factorize in
> > this case, the same comments I made above should apply here.
> 
> The difference is when the pretty printer is turned on, actually. The
> "no_pretty" option doesn't enter the frame that creates our problems,
> just turns the pretty printer on and waits by the door - setup for
> step and continue, whereas the "pretty" option enters the frame and
> then turns the pretty printer on - setup for backtrace, finish, up and
> down.
> 
> I'll change the function to receive the breaking location instead.
> Sorry about the confusion.

Alright! Thanks for the explanation.


-- 
Joel