Re: [PATCH v3 3/5] Add gdb::string_view

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2018-04-09 09:44, Pedro Alves wrote:
> On 04/07/2018 02:26 PM, Simon Marchi wrote:
> > New in v3:
> >
> >   * Base on the implementation in experimental/, since it is easier to
> >   adapt to the c++ standard we follow (c++11).  I didn't see any
> >   significant fix to import from the std/ version.
> >
> >   * Tested against clang and clang+libc++ to make sure we don't rely 
> > on
> >   anything internal and specific to libstdc++.  This caught a few
> >   issues.
> >
> >   * Remove hash helpers, operator<<, operator ""sv.
>
> We can always add hash support later, but I suspect it won't be
> long until we will miss it, when we try to use string_view with
> e.g., an std::unordered_map.

Ok.  I'd still like to keep it out of this patch, as it will probably be 
a bit involved.

> >   * Make gdb::string_view an alias of std::string_view when building
> >   with >= c++17.
> >
> >   * Remove a bunch of constexpr, because they are not valid in c++11
> >   (e.g. they are not a single return line).
>
> I think I'd prefer to leave the "constexpr"'s in place, but commented 
> out,
> instead of just removing it, like '/*constexpr*/', as a clearer marker 
> that
> we've removed something that normally would otherwise be there, since 
> for
> a future reader, simply removing is indistinguishable with 
> missing/forgetting
> a constexpr.

Done.

> >   * Remove hash helpers, because they use libstdc++ internal 
> > functions.
> >   If we need them we always import them later.
>
> Ah, hash helpers mentioned twice. :-)

Woops.

> Technically, the use of uglified (double-underscore or
> underscore-followed-by-uppercase) names is undefined behavior for
> being reserved to the implementation, but I think that in practice
> it's harmless, we should be able to deal with any conflict if it
> ever happens.  At this stage, I think fewer local changes outweighs
> the very small potential for trouble.
>
> LGTM.

Thanks!

Simon