From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10284 invoked by alias); 11 Jan 2012 18:32:28 -0000 Received: (qmail 10276 invoked by uid 22791); 11 Jan 2012 18:32:27 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-vx0-f169.google.com (HELO mail-vx0-f169.google.com) (209.85.220.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 11 Jan 2012 18:32:09 +0000 Received: by vcge1 with SMTP id e1so867295vcg.0 for ; Wed, 11 Jan 2012 10:32:09 -0800 (PST) Received: by 10.220.153.134 with SMTP id k6mr288122vcw.23.1326306729174; Wed, 11 Jan 2012 10:32:09 -0800 (PST) MIME-Version: 1.0 Received: by 10.220.3.130 with HTTP; Wed, 11 Jan 2012 10:31:48 -0800 (PST) In-Reply-To: <94906C8E-C23D-4DA3-989D-DDCCFA20FC35@cs.umd.edu> References: <94906C8E-C23D-4DA3-989D-DDCCFA20FC35@cs.umd.edu> From: Kevin Pouget Date: Wed, 11 Jan 2012 18:48:00 -0000 Message-ID: Subject: Re: Make the "python" command resemble the standard Python interpreter To: Khoo Yit Phang Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-01/txt/msg00360.txt.bz2 On Wed, Jan 11, 2012 at 4:53 PM, Khoo Yit Phang 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 downl= oad the attachment too. Perhaps a email formatting glitch? yes, if you're sure about the indentation, forget what I said ;) >>> +{ >>> + =A0int n; >>> + =A0char *p =3D NULL, *p_start, *p_end, *q; >>> + =A0volatile struct gdb_exception except; >>> + >>> + =A0TRY_CATCH (except, RETURN_MASK_ALL) >>> + =A0 =A0{ >>> + =A0 =A0 =A0struct cleanup *cleanup =3D 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 =3D gdbpy_suspend_sigint_handler (); > + p =3D command_line_input (prompt, 0, "python"); >>> + =A0 =A0 =A0p =3D command_line_input (prompt, 0, "python"); >>> + =A0 =A0 =A0do_cleanups (cleanup); >>> + =A0 =A0} >> >> 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 hand= le 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. >>> + =A0/* Detect Ctrl-C and treat as KeyboardInterrupt. */ >>> + =A0if (except.reason =3D=3D RETURN_QUIT) >>> + =A0 =A0return NULL; >>> + >>> + =A0/* Handle errors by raising Python exceptions. */ >>> + =A0if (except.reason < 0) >>> + =A0 =A0{ >>> + =A0 =A0 =A0/* The thread state is nulled during gdbpy_readline_wrappe= r, >>> + 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. > >>> +{ >>> + =A0Py_InitModule3 ("readline", GdbReadlineMethods, >>> + =A0"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 =3D add_com ("tbreak", class_breakpoint, tbreak_command, _("\ > Set a temporary breakpoint.\n\ > Like \"break\" except the breakpoint is only temporary,\n\ > ... Kevin