This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA 1/5] Remove some ui_out-related cleanups from Python


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]