[PATCH] Remove some variables in favor of using gdb::optional
Ruslan Kabatsayev
b7.10110111@gmail.com
Sat Aug 24 11:23:00 GMT 2019
Hi,
On Fri, 23 Aug 2019 at 22:33, Simon Marchi <simon.marchi@polymtl.ca> wrote:
>
> On 2019-08-23 11:35 a.m., Pedro Alves wrote:
> >> Boost is very widely available, so I'd prefer if we could depend on the system-installed
> >> boost, or have the user provide it, but I would understand if others didn't want to add
> >> that burden to those who build GDB.
> >
> > Yeah, boost would be a too-big dependency, IMO. It's huge.
>
> True, libbost1.67-dev on Debian unpacks to 150 MB of header files. It would be nice to be able to
> select which features one actually need and check-in just the necessary files (a bit like gnulib).
> I bet that for tribool, it would be just a few files.
>
> >
> >> So an alternative would be to carry a version of boost
> >> (maybe just the files that we need) in our repo. This would have the advantage that everybody
> >> would build with the same version, hence less chances of build failures with particular
> >> combinations of compilers and boost versions, or using features from a too recent boost.
> >>
> >> What do you think?
> >
> > I would really not like to carry boost. It's very big and intertwined. I think the
> > costs outweighs the benefits here, because we'd be pulling in a huge codebase when we
> > only need minimal things. I'd also like to move more in the direction of sharing more
> > code across the toolchain (gdb, gcc, gold, etc.) and depending on boost would
> > certainly prevent that. LLVM doesn't depend on boost either, for example. Given
> > that it's a C++ project from the get go, I think that's telling. Most certainly for
> > the same reasons.
> >
> > On reusing parts, when I was working on the original gdb::unique_ptr emulation for
> > C++98, before we upgraded to C++11, I looked at reusing boost's version. What I
> > found was a huge horrible mess, with lots of incomprehensible #ifdefery to support
> > compilers that no one cares about anymore (old Borland C++, etc.), impossible to
> > decouple. Another issue is that a boost API may have been originally designed
> > with C++98 in mind, and it'd be done differently with C++11 and later in mind.
> > It might have been the case here, for example note how boost's tribool uses
> > the C++98 safe bool idiom, instead of C++11 operator bool().
>
> Ok, all good points.
>
> >> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> >> index de9755f6ce30..915e0b25cea1 100644
> >> --- a/gdb/dwarf2read.c
> >> +++ b/gdb/dwarf2read.c
> >> @@ -90,6 +90,7 @@
> >> #include <forward_list>
> >> #include "rust-lang.h"
> >> #include "gdbsupport/pathstuff.h"
> >> +#include <boost/logic/tribool.hpp>
> >>
> >> /* When == 1, print basic high level tracing messages.
> >> When > 1, be more verbose.
> >> @@ -5843,7 +5844,7 @@ dw2_debug_names_iterator::next ()
> >> return NULL;
> >> }
> >> const mapped_debug_names::index_val &indexval = indexval_it->second;
> >> - gdb::optional<bool> is_static;
> >> + boost::tribool is_static(boost::indeterminate);
> >
> > Here I imagine we'd more naturally want to treat a tribool like
> > a built-in, similar to an enum class that accepts {true, false, unknown},
> > so we'd write:
> >
> > tribool is_static = true;
> > tribool is_static = false;
> > tribool is_static = tribool::unknown;
>
> Yes that sounds good.
>
> >
> > ("indeterminate" is really a mouthful, hence "unknown".)
> I also wasn't sure about this, what the third state should be, more on that below.
>
> > with tribool::unknown being e.g., a constexpr global.
> >> dwarf2_per_cu_data *per_cu = NULL;
> >> for (const mapped_debug_names::index_val::attr &attr : indexval.attr_vec)
> >> {
> >> @@ -5910,10 +5911,10 @@ dw2_debug_names_iterator::next ()
> >> goto again;
> >>
> >> /* Check static vs global. */
> >> - if (is_static.has_value () && m_block_index.has_value ())
> >> + if (!boost::indeterminate(is_static) && m_block_index.has_value ())
> >
> > and here:
> >
> > + if (is_static != tribool::unknown && m_block_index.has_value ())
> >
> > Here's a quick-prototype starter implementation. It's missing sprinkling
> > with constexpr, inline, noexcept, relational operators (op|| and op&& ?),
> > unit tests, etc., but should show the idea.
> >
> > #include <stdio.h>
> >
> > class tribool
> > {
> > private:
> > struct unknown_t {};
> > public:
> > /* Keep it trivial. */
> > tribool () = default;
> >
> > tribool (bool val)
> > : m_value (val)
> > {}
> >
> > operator bool () const
> > {
> > return m_value == 1;
> > }
> >
> > bool operator== (const tribool &other)
> > {
> > return m_value == other.m_value;
> > }
> >
> > tribool operator! ()
> > {
> > if (m_value == 0)
> > return true;
> > else if (m_value == 1)
> > return false;
> > else
> > return unknown;
> > }
> >
> > static tribool unknown;
> >
> > private:
> > tribool (unknown_t dummy)
> > : m_value (-1)
> > {}
> >
> > int m_value; // -1 for unknown.
> > };
> >
> > tribool tribool::unknown (unknown_t{});
> >
> > int
> > main ()
> > {
> > tribool b1 = true;
> > tribool b2 = false;
> > tribool b3 = tribool::unknown;
> > tribool b = b3;
> >
> > if (b)
> > printf ("then\n");
> > else if (!b)
> > printf ("else\n");
> > else
> > printf ("unknown\n");
> >
> > return b1;
> > }
> >
> > Would you like to run with this?
>
> So I wasn't sure about what the third state should be. I think it depends on
> the particular context. In different contexts, it could mean "unknown", "unspecified",
> "auto", "don't care", etc. There's no one-size word that fits all case, so I don't really
> like the idea of having just one word and have it represent poorly what we actually mean.
>
> That lead me to think, if we want to represent three states and if the states are
> specific to each use case, why not just define an enum and be explicit about it?
>
> A bit like why I prefer defining an explicit type with two fields rather than using
> std::pair: the "first" and "second" members are not very descriptive.
>
> Here's a patch that does that. What do you think?
>
>
> From ba180d647f6ceb69b9a01c46807c102439b8cae1 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Fri, 23 Aug 2019 14:53:59 -0400
> Subject: [PATCH] dwarf2read: replace gdb::optional<bool> with enum
>
> gdb::optional<bool> is dangerous, because it's easy to do:
>
> if (opt_bool)
>
> when you actually meant
>
> if (*opt_bool)
>
> or vice-versa. The first checks if the optional is set, the second
> checks if the wrapped bool is true.
>
> Replace it with an enum that explicitly defines the three possible
> states.
>
> gdb/ChangeLog:
>
> * dwarf2read.c (dw2_debug_names_iterator::next): Use enum to
> represent whether the symbol is static, dynamic, or we don't
> know.
> ---
> gdb/dwarf2read.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index de9755f6ce30..3dc34ab5a339 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -5843,7 +5843,11 @@ dw2_debug_names_iterator::next ()
> return NULL;
> }
> const mapped_debug_names::index_val &indexval = indexval_it->second;
> - gdb::optional<bool> is_static;
> + enum {
> + symbol_nature_unknown,
> + symbol_nature_static,
> + symbol_nature_dynamic,
> + } symbol_nature = symbol_nature_unknown;
Since GDB uses C++11, and we don't really rely on conversion to int
here, why not use enum class? This would protect from unwanted
conversions. Additionally, we'll be forced to use a "scoped" name
instead of "prefixed" one, like symbol_nature::unknown vs
symbol_nature_unknown, which seems a bit more explicit (will need to
do something with "static" though, since it's a keyword).
> dwarf2_per_cu_data *per_cu = NULL;
> for (const mapped_debug_names::index_val::attr &attr : indexval.attr_vec)
> {
> @@ -5895,12 +5899,12 @@ dw2_debug_names_iterator::next ()
> case DW_IDX_GNU_internal:
> if (!m_map.augmentation_is_gdb)
> break;
> - is_static = true;
> + symbol_nature = symbol_nature_static;
> break;
> case DW_IDX_GNU_external:
> if (!m_map.augmentation_is_gdb)
> break;
> - is_static = false;
> + symbol_nature = symbol_nature_dynamic;
> break;
> }
> }
> @@ -5910,10 +5914,11 @@ dw2_debug_names_iterator::next ()
> goto again;
>
> /* Check static vs global. */
> - if (is_static.has_value () && m_block_index.has_value ())
> + if (symbol_nature != symbol_nature_unknown && m_block_index.has_value ())
> {
> const bool want_static = *m_block_index == STATIC_BLOCK;
> - if (want_static != *is_static)
> + const bool symbol_is_static = symbol_nature == symbol_nature_static;
> + if (want_static != symbol_is_static)
> goto again;
> }
>
> --
> 2.23.0
>
Regards,
Ruslan
More information about the Gdb-patches
mailing list