Re: Make the "python" command resemble the standard Python interpreter

[Kevin Pouget <kevin dot pouget at gmail dot com>]

On Wed, Jan 11, 2012 at 4:53 PM, Khoo Yit Phang <khooyp@cs.umd.edu> wrote:
> Hi,
>
> Thanks for the review, I'll attach an updated patch in a moment. I have a few questions:
>
> On Jan 11, 2012, at 5:26 AM, Kevin Pouget wrote:
>
>>> +static char *
>>> +gdbpy_readline_wrapper (FILE *sys_stdin, FILE *sys_stdout,
>>> + char *prompt)
>>
>> I think that 'char *prompt' whould be aligned with FILE *sys_stdin'
>
> It is properly tabbed originally and it seems to be the case when I download the attachment too. Perhaps a email formatting glitch?

yes, if you're sure about the indentation, forget what I said ;)

>>> +{
>>> + ?int n;
>>> + ?char *p = NULL, *p_start, *p_end, *q;
>>> + ?volatile struct gdb_exception except;
>>> +
>>> + ?TRY_CATCH (except, RETURN_MASK_ALL)
>>> + ? ?{
>>> + ? ? ?struct cleanup *cleanup = gdbpy_suspend_sigint_handler ();
>>
>> new line between declarations and the code
>
> Do you mean there should not be a new line?

no, you _should_ have a line between these two lines:

> +      struct cleanup *cleanup = gdbpy_suspend_sigint_handler ();
> +      p = command_line_input (prompt, 0, "python");

>>> + ? ? ?p = command_line_input (prompt, 0, "python");
>>> + ? ? ?do_cleanups (cleanup);
>>> + ? ?}
>>
>> I'm not sure about that, but isn't the clean up supposed to be
>> executed even if an exception is thrown? it seems not to be the case
>> here
>
> Are you referring to do_cleanups? If I understand correctly, it's to handle the case where an exception is not thrown (see, e.g., py-value.c).

I think that's you're supposed to use the cleanup machinery when you
don't explicitely handle the exception. Here you code looks like:

> TRY_CATCH
> {
>   do_something_dangerous()
> }
> handle_exception_if_any()
> continue_anyway()

so I think it's safe to simply call "gdbpy_suspend_sigint_handler"
after the exception handling.

>>> + ?/* Detect Ctrl-C and treat as KeyboardInterrupt. */
>>> + ?if (except.reason == RETURN_QUIT)
>>> + ? ?return NULL;
>>> +
>>> + ?/* Handle errors by raising Python exceptions. */
>>> + ?if (except.reason < 0)
>>> + ? ?{
>>> + ? ? ?/* The thread state is nulled during gdbpy_readline_wrapper,
>>> + with the original value saved in the following undocumented
>>> + variable (see Python's Parser/myreadline.c and
>>> + Modules/readline.c). */
>>
>> comment lines should be aligned with "The thread" (or maybe my tabs
>> are not displayed properly)
>
> That's might be the case.
>
>>> +{
>>> + ?Py_InitModule3 ("readline", GdbReadlineMethods,
>>> + ?"GDB-provided dummy readline module to prevent conflicts with the standard readline module.");
>>
>> This line is too long, should be < 80 chars
>
> I'll shorten the line, but separately, how do you format lines containing strings that themselves can be up to 80 chars (e.g., for printing)?

you can use the C line breaker \,  there're examples at the end of
many .c and py-*.c files, like

>  c = add_com ("tbreak", class_breakpoint, tbreak_command, _("\
> Set a temporary breakpoint.\n\
> Like \"break\" except the breakpoint is only temporary,\n\
> ...


Kevin