public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Doug Evans <xdje42@gmail.com>
To: Siva Chandra <sivachandra@google.com>
Cc: gdb-patches <gdb-patches@sourceware.org>
Subject: Re: [RFC/Patch] Call overloaded operators to perform valid Python operations on struct/class values.
Date: Fri, 06 Dec 2013 06:25:00 -0000	[thread overview]
Message-ID: <m31u1q1oq0.fsf@sspiff.org> (raw)
In-Reply-To: <CAGyQ6gxk9zcLKQ1Ef2XhgUMCyB8MB1v=6tA4jsfPyEFgxR8bNw@mail.gmail.com>	(Siva Chandra's message of "Mon, 2 Dec 2013 11:28:42 -0800")

Siva Chandra <sivachandra@google.com> writes:
> Hi,
>
> This is a follow up to this thread:
> https://sourceware.org/ml/gdb/2013-11/msg00101.html
>
> Part of Doug's response there seemed to indicate that calling
> overloaded operators to perform valid Python operations has not yet
> been implemented. The attached patch adds this "feature". I could not
> yet think of a reason as to why adding this could be bad (as in,
> leading to ambiguity or something similar).
>
> ChangeLog
>
> 2013-12-02  Siva Chandra Reddy  <sivachandra@google.com>
>
>         Call overloaded operators to perform valid Python operations on
>         struct/class values.
>
>         * python/py-value.c (valpy_binop): Call value_x_binop for struct
>         and class values.
>
>         testsuite/
>         * gdb.python/py-value-cc.cc: Improve test case.
>         * gdb.python/py-value-cc.exp: Add new tests.

Hi.

I'm not yet comfortable enough with going this route to approve it.
It feels sexy and all, but it's not clear to me going this path is a net win.
If we could release it as experimental, without any promises to keep it
or change it in incompatible ways, I'd say go for it.
I'm curious what others think.

Nits on the patch itself:

1) In the changelog entry, no blank line here.

>         Call overloaded operators to perform valid Python operations on
>         struct/class values.
>  <<<no blank line here>>>
>         * python/py-value.c (valpy_binop): Call value_x_binop for struct
>         and class values.

2) NEWS entry and doc required.
I can well imagine that they're coming, pending approval of the patch
thus far.  Sorry!

3) gdb code uses tabs for indentation (and then uses spaces after the
needed amount of tabs have been used), and it's a pain when tabbed
code is mixed with non-tabbed code.

4) In the test case, don't make extra changes, or if you do split
them out into a separate patch.

  reply	other threads:[~2013-12-06  6:25 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-02 19:28 Siva Chandra
2013-12-06  6:25 ` Doug Evans [this message]
2013-12-06 14:20   ` Siva Chandra
2013-12-11 20:18     ` Tom Tromey
2013-12-11 20:17   ` Tom Tromey
2013-12-16  7:48     ` Doug Evans
2013-12-16 22:24       ` Siva Chandra
2013-12-18 16:37         ` Doug Evans
2013-12-18 23:15           ` Siva Chandra
2013-12-19 14:11             ` Doug Evans
2013-12-19 17:50               ` Siva Chandra
2013-12-20 22:29                 ` Siva Chandra
2013-12-21  8:21                   ` Eli Zaretskii
2013-12-30 14:40                     ` Siva Chandra
2013-12-30 17:57                       ` Eli Zaretskii
2014-01-22 21:39                       ` Siva Chandra
2014-01-25 18:45                         ` Doug Evans

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m31u1q1oq0.fsf@sspiff.org \
    --to=xdje42@gmail.com \
    --cc=gdb-patches@sourceware.org \
    --cc=sivachandra@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).