Mirror of the gdb-patches mailing list
 help / color / mirror / Atom feed
From: Pedro Alves <palves@redhat.com>
To: Trevor Saunders <tbsaunde@tbsaunde.org>, Tom Tromey <tom@tromey.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [RFA 1/5] Remove some ui_out-related cleanups from Python
Date: Wed, 08 Feb 2017 17:28:00 -0000	[thread overview]
Message-ID: <c0a70339-49c3-a087-bebf-ecfdd2cc32ca@redhat.com> (raw)
In-Reply-To: <20170116113021.sar3yh5ivykpqmbw@ball>

On 01/16/2017 11:30 AM, Trevor Saunders wrote:
>> +++ b/gdb/common/gdb_option.h
> 
> might be nice to put it in include/ but fine to do that later when
> something else actually wants it.

I've been thinking about putting all these utilities and
later-standards replacements we're coming up with in its own
directory/namespace.  I had thought of "gtl", for "gdb template
library", or "gnu template library" if other projects want to
reuse it.  (I think "gnu" is kind of taken by gnulib / libgnu.a)
One difficulty with putting such a thing at the top level is that
gdb relies on gnulib headers (that exist under gdb/) while
other toolchain components don't, yet, at least.

>> > +  /* These aren't deleted in std::optional, but it was simpler to
>> > +     delete them here, because currently the users of this class don't
>> > +     need them, and making them depend on the definition of T is
>> > +     somewhat complicated.  */
> I think you can make <type_traits> do most of it, but fair enough.
> 

Back around <https://sourceware.org/ml/gdb-patches/2016-11/msg00368.html>
I backported C++17's optional to C++11.  I still have that branch
somewhere locally...  /me finds it and pushes to github.  Here:

 https://github.com/palves/gdb/commits/palves/gdb-import-gcc-optional

This <https://github.com/palves/gdb/commit/880ab485c5873eb0a8ab1dca75e624ec2a064fe8>
contains the local changes I had made to to port it to C++11.  I was surprised
that other than portable type traits stuff that's missing in C++11, the
only thing that C++11 loses is constexpr-ness in some cases.  Now,
the result is of course much more code than what Tromey is proposing.
It probably does make sense to start with something simpler and
upgrade when/if we find a need.  OTOH, looks like the current patch
doesn't have accessors for the wrapped value, so it seems like we'll
at least need to be extend it in that direction in no time.

>> +  /* True if the object was ever emplaced.  */
>> +  bool m_instantiated;
>> +
>> +  /* The object.  */
>> +  union
>> +  {
>> +    struct { } m_dummy;
>> +    T m_item;
>> +  };
> 
> It doesn't matter yet, but space wise it would be better to put the bool
> last right? For example if sizeof(T) is 6, or if the optional is in some
> larger structure with a bool next.

Agreed.  Seems like that's what gcc's optional does.

I also agree with Simon's suggestion for renaming the file.

Patch LGTM with Trevor's and Simon's nits  addressed.

I wondered about making m_instantiated a char, so that optional<T> would
pack even better when sizeof or alignof T is small, and thus ends up being
no cost in those cases space-wise.  Though maybe that ends up being
worse / not so efficient generated code -wise, or GCC would do it too?
In any case, since GCC doesn't do that, if/when we ever move to C++17,
we'd lose that again anyway.

Thanks,
Pedro Alves


  reply	other threads:[~2017-02-08 17:28 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-01-15 13:43 [RFA 0/5] more cleanup removal in Python Tom Tromey
2017-01-15 13:43 ` [RFA 1/5] Remove some ui_out-related cleanups from Python Tom Tromey
2017-01-15 21:52   ` Simon Marchi
2017-01-16 16:13     ` Tom Tromey
2017-01-16 11:19   ` Trevor Saunders
2017-02-08 17:28     ` Pedro Alves [this message]
2017-02-08 22:27       ` Pedro Alves
2017-02-08 23:05       ` Tom Tromey
2017-02-08 23:52         ` Pedro Alves
2017-02-09  4:34           ` Matt Rice
2017-02-09 12:48             ` Pedro Alves
2017-02-09 12:51               ` Pedro Alves
2017-02-09 15:46                 ` Matt Rice
2017-02-09 16:04                   ` Simon Marchi
2017-02-10  6:47       ` Trevor Saunders
2017-01-15 13:43 ` [RFA 3/5] Introduce gdbpy_subclass and use it to simplify some logic Tom Tromey
2017-01-24 20:21   ` Simon Marchi
2017-02-09 11:44     ` Pedro Alves
2017-02-09 18:52       ` Tom Tromey
2017-02-09 13:00   ` Pedro Alves
2017-01-15 13:43 ` [RFA 4/5] Change one more spot to use gdbpy_ref Tom Tromey
2017-02-09 12:52   ` Pedro Alves
2017-01-15 13:43 ` [RFA 5/5] Remove some gotos from Python Tom Tromey
2017-02-09 13:03   ` Pedro Alves
2017-01-15 13:43 ` [RFA 2/5] Introduce ui_file_up and use it to remove cleanups Tom Tromey
2017-01-16  9:59   ` Trevor Saunders
2017-01-16 17:58   ` Pedro Alves
2017-01-16 19:08     ` Tom Tromey
2017-01-17  1:40       ` Pedro Alves
2017-01-17 19:05         ` Tom Tromey

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=c0a70339-49c3-a087-bebf-ecfdd2cc32ca@redhat.com \
    --to=palves@redhat.com \
    --cc=gdb-patches@sourceware.org \
    --cc=tbsaunde@tbsaunde.org \
    --cc=tom@tromey.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