[PATCH] Remove some variables in favor of using gdb::optional

[PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

While reading that code, I noticed that some variables essentially meant
whether to consider some other variable or not.  I think using
gdb::optional (which was not available when this code was written) is
clearer, as it embeds the used/not used predicate directly in the type
of the variable, making it harder to miss.

Reg-tested on the buildbot.

gdb/ChangeLog:

	* dwarf2read.c (struct dw2_symtab_iterator):
	<want_specific_block>: Remove.
	<block_index>: Change type to gdb::optional.
	(dw2_symtab_iter_init): Remove WANT_SPECIFIC_BLOCK parameter,
	change type of BLOCK_INDEX parameter to gdb::optional.
	(dw2_symtab_iter_next): Re-write in function of gdb::optional.
	(dw2_lookup_symbol): Don't pass argument for
	WANT_SPECIFIC_BLOCK.
	(dw2_expand_symtabs_for_function): Don't pass argument for
	WANT_SPECIFIC_BLOCK, pass empty optional for BLOCK_INDEX.
	(class dw2_debug_names_iterator)
	<dw2_debug_names_iterator>: Remove WANT_SPECIFIC_BLOCK
	parameter, change BLOCK_INDEX type to gdb::optional.
	<m_want_specific_block>: Remove.
	<m_block_index>: Change type to gdb::optional.
	(dw2_debug_names_iterator::next): Change type of IS_STATIC to
	gdb::optional.  Re-write in function of gdb::optional.
	(dw2_debug_names_lookup_symbol): Don't pass argument for
	WANT_SPECIFIC_BLOCK.
	(dw2_debug_names_expand_symtabs_for_function): Don't pass
	argument for WANT_SPECIFIC_BLOCK, pass empty optional for
	BLOCK_INDEX.
---
 gdb/dwarf2read.c | 77 +++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 47 deletions(-)

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 3d90d6328917..7466d1538b58 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -3881,11 +3881,9 @@ struct dw2_symtab_iterator
 {
   /* The dwarf2_per_objfile owning the CUs we are iterating on.  */
   struct dwarf2_per_objfile *dwarf2_per_objfile;
-  /* If non-zero, only look for symbols that match BLOCK_INDEX.  */
-  int want_specific_block;
-  /* One of GLOBAL_BLOCK or STATIC_BLOCK.
-     Unused if !WANT_SPECIFIC_BLOCK.  */
-  int block_index;
+  /* If set, only look for symbols that match that block.  Valid values are
+     GLOBAL_BLOCK and STATIC_BLOCK.  */
+  gdb::optional<int> block_index;
   /* The kind of symbol we're looking for.  */
   domain_enum domain;
   /* The list of CUs from the index entry of the symbol,
@@ -3902,20 +3900,16 @@ struct dw2_symtab_iterator
   int global_seen;
 };
 
-/* Initialize the index symtab iterator ITER.
-   If WANT_SPECIFIC_BLOCK is non-zero, only look for symbols
-   in block BLOCK_INDEX.  Otherwise BLOCK_INDEX is ignored.  */
+/* Initialize the index symtab iterator ITER.  */
 
 static void
 dw2_symtab_iter_init (struct dw2_symtab_iterator *iter,
 		      struct dwarf2_per_objfile *dwarf2_per_objfile,
-		      int want_specific_block,
-		      int block_index,
+		      gdb::optional<int> block_index,
 		      domain_enum domain,
 		      const char *name)
 {
   iter->dwarf2_per_objfile = dwarf2_per_objfile;
-  iter->want_specific_block = want_specific_block;
   iter->block_index = block_index;
   iter->domain = domain;
   iter->next = 0;
@@ -3945,9 +3939,6 @@ dw2_symtab_iter_next (struct dw2_symtab_iterator *iter)
       offset_type cu_index_and_attrs =
 	MAYBE_SWAP (iter->vec[iter->next + 1]);
       offset_type cu_index = GDB_INDEX_CU_VALUE (cu_index_and_attrs);
-      int want_static = iter->block_index != GLOBAL_BLOCK;
-      /* This value is only valid for index versions >= 7.  */
-      int is_static = GDB_INDEX_SYMBOL_STATIC_VALUE (cu_index_and_attrs);
       gdb_index_symbol_kind symbol_kind =
 	GDB_INDEX_SYMBOL_KIND_VALUE (cu_index_and_attrs);
       /* Only check the symbol attributes if they're present.
@@ -3977,9 +3968,16 @@ dw2_symtab_iter_next (struct dw2_symtab_iterator *iter)
       /* Check static vs global.  */
       if (attrs_valid)
 	{
-	  if (iter->want_specific_block
-	      && want_static != is_static)
-	    continue;
+	  bool is_static = GDB_INDEX_SYMBOL_STATIC_VALUE (cu_index_and_attrs);
+
+	  if (iter->block_index.has_value ())
+	    {
+	      bool want_static = *iter->block_index == STATIC_BLOCK;
+
+	      if (is_static != want_static)
+		continue;
+	    }
+
 	  /* Work around gold/15646.  */
 	  if (!is_static && iter->global_seen)
 	    continue;
@@ -4032,7 +4030,7 @@ dw2_lookup_symbol (struct objfile *objfile, int block_index,
   struct dw2_symtab_iterator iter;
   struct dwarf2_per_cu_data *per_cu;
 
-  dw2_symtab_iter_init (&iter, dwarf2_per_objfile, 1, block_index, domain, name);
+  dw2_symtab_iter_init (&iter, dwarf2_per_objfile, block_index, domain, name);
 
   while ((per_cu = dw2_symtab_iter_next (&iter)) != NULL)
     {
@@ -4115,9 +4113,7 @@ dw2_expand_symtabs_for_function (struct objfile *objfile,
   struct dw2_symtab_iterator iter;
   struct dwarf2_per_cu_data *per_cu;
 
-  /* Note: It doesn't matter what we pass for block_index here.  */
-  dw2_symtab_iter_init (&iter, dwarf2_per_objfile, 0, GLOBAL_BLOCK, VAR_DOMAIN,
-			func_name);
+  dw2_symtab_iter_init (&iter, dwarf2_per_objfile, {}, VAR_DOMAIN, func_name);
 
   while ((per_cu = dw2_symtab_iter_next (&iter)) != NULL)
     dw2_instantiate_symtab (per_cu, false);
@@ -5661,14 +5657,11 @@ dwarf2_read_debug_names (struct dwarf2_per_objfile *dwarf2_per_objfile)
 class dw2_debug_names_iterator
 {
 public:
-  /* If WANT_SPECIFIC_BLOCK is true, only look for symbols in block
-     BLOCK_INDEX.  Otherwise BLOCK_INDEX is ignored.  */
   dw2_debug_names_iterator (const mapped_debug_names &map,
-			    bool want_specific_block,
-			    block_enum block_index, domain_enum domain,
+			    gdb::optional<block_enum> block_index,
+			    domain_enum domain,
 			    const char *name)
-    : m_map (map), m_want_specific_block (want_specific_block),
-      m_block_index (block_index), m_domain (domain),
+    : m_map (map), m_block_index (block_index), m_domain (domain),
       m_addr (find_vec_in_debug_names (map, name))
   {}
 
@@ -5691,13 +5684,9 @@ private:
   /* The internalized form of .debug_names.  */
   const mapped_debug_names &m_map;
 
-  /* If true, only look for symbols that match BLOCK_INDEX.  */
-  const bool m_want_specific_block = false;
-
-  /* One of GLOBAL_BLOCK or STATIC_BLOCK.
-     Unused if !WANT_SPECIFIC_BLOCK - FIRST_LOCAL_BLOCK is an invalid
-     value.  */
-  const block_enum m_block_index = FIRST_LOCAL_BLOCK;
+  /* If set, only look for symbols that match that block.  Valid values are
+     GLOBAL_BLOCK and STATIC_BLOCK.  */
+  const gdb::optional<block_enum> m_block_index;
 
   /* The kind of symbol we're looking for.  */
   const domain_enum m_domain = UNDEF_DOMAIN;
@@ -5854,8 +5843,7 @@ dw2_debug_names_iterator::next ()
       return NULL;
     }
   const mapped_debug_names::index_val &indexval = indexval_it->second;
-  bool have_is_static = false;
-  bool is_static;
+  gdb::optional<bool> is_static;
   dwarf2_per_cu_data *per_cu = NULL;
   for (const mapped_debug_names::index_val::attr &attr : indexval.attr_vec)
     {
@@ -5907,13 +5895,11 @@ dw2_debug_names_iterator::next ()
 	case DW_IDX_GNU_internal:
 	  if (!m_map.augmentation_is_gdb)
 	    break;
-	  have_is_static = true;
 	  is_static = true;
 	  break;
 	case DW_IDX_GNU_external:
 	  if (!m_map.augmentation_is_gdb)
 	    break;
-	  have_is_static = true;
 	  is_static = false;
 	  break;
 	}
@@ -5924,11 +5910,11 @@ dw2_debug_names_iterator::next ()
     goto again;
 
   /* Check static vs global.  */
-  if (have_is_static)
+  if (is_static.has_value () && m_block_index.has_value ())
     {
-      const bool want_static = m_block_index != GLOBAL_BLOCK;
-      if (m_want_specific_block && want_static != is_static)
-	goto again;
+	const bool want_static = *m_block_index == STATIC_BLOCK;
+	if (want_static != *is_static)
+	  goto again;
     }
 
   /* Match dw2_symtab_iter_next, symbol_kind
@@ -6027,8 +6013,7 @@ dw2_debug_names_lookup_symbol (struct objfile *objfile, int block_index_int,
     }
   const auto &map = *mapp;
 
-  dw2_debug_names_iterator iter (map, true /* want_specific_block */,
-				 block_index, domain, name);
+  dw2_debug_names_iterator iter (map, block_index, domain, name);
 
   struct compunit_symtab *stab_best = NULL;
   struct dwarf2_per_cu_data *per_cu;
@@ -6091,9 +6076,7 @@ dw2_debug_names_expand_symtabs_for_function (struct objfile *objfile,
     {
       const mapped_debug_names &map = *dwarf2_per_objfile->debug_names_table;
 
-      /* Note: It doesn't matter what we pass for block_index here.  */
-      dw2_debug_names_iterator iter (map, false /* want_specific_block */,
-				     GLOBAL_BLOCK, VAR_DOMAIN, func_name);
+      dw2_debug_names_iterator iter (map, {}, VAR_DOMAIN, func_name);
 
       struct dwarf2_per_cu_data *per_cu;
       while ((per_cu = iter.next ()) != NULL)
-- 
2.22.0

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> While reading that code, I noticed that some variables essentially meant
Simon> whether to consider some other variable or not.  I think using
Simon> gdb::optional (which was not available when this code was written) is
Simon> clearer, as it embeds the used/not used predicate directly in the type
Simon> of the variable, making it harder to miss.

Thanks, this looks reasonable to me.

Simon> +  /* If set, only look for symbols that match that block.  Valid values are
Simon> +     GLOBAL_BLOCK and STATIC_BLOCK.  */
Simon> +  gdb::optional<int> block_index;

I guess optional<block_enum> would be even better here.

Tom

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-04 5:21 p.m., Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> While reading that code, I noticed that some variables essentially meant
> Simon> whether to consider some other variable or not.  I think using
> Simon> gdb::optional (which was not available when this code was written) is
> Simon> clearer, as it embeds the used/not used predicate directly in the type
> Simon> of the variable, making it harder to miss.
> 
> Thanks, this looks reasonable to me.

Thanks, I'll push it momentarily.

> Simon> +  /* If set, only look for symbols that match that block.  Valid values are
> Simon> +     GLOBAL_BLOCK and STATIC_BLOCK.  */
> Simon> +  gdb::optional<int> block_index;
> 
> I guess optional<block_enum> would be even better here.

Yeah, I started doing this, but then thought I would do it as a separate patch, changing
the quick_symbol_functions::lookup_symbol interface, and all it impacts.

Simon

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Yeah, I started doing this, but then thought I would do it as a
Simon> separate patch, changing the
Simon> quick_symbol_functions::lookup_symbol interface, and all it
Simon> impacts.

Ouch, yeah, that makes sense.

Tom

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Pedro Alves]

On 8/4/19 9:10 PM, Simon Marchi wrote:
> -  bool have_is_static = false;
> -  bool is_static;
> +  gdb::optional<bool> is_static;

std::optional<bool> is evil.  :-)

E.g. it's very easy to write (or miss converting)

 if (is_static)

and not realize that that is doing the wrong thing.

https://www.boost.org/doc/libs/1_57_0/libs/optional/doc/html/boost_optional/tutorial/a_note_about_optional_bool_.html

Not saying to change the code (*), but seeing it gives me the creeps, so I couldn't resist.  :-)

* - a yes/no/unknown tristate type a-la boost::tribool would be nice
    to have, IMO.  It could be used more clearly in these situations
    and could supersede auto_boolean.

Thanks,
Pedro Alves

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-21 3:38 p.m., Pedro Alves wrote:
> std::optional<bool> is evil.  :-)
> 
> E.g. it's very easy to write (or miss converting)
> 
>  if (is_static)
> 
> and not realize that that is doing the wrong thing.
> 
> https://www.boost.org/doc/libs/1_57_0/libs/optional/doc/html/boost_optional/tutorial/a_note_about_optional_bool_.html
> 
> Not saying to change the code (*), but seeing it gives me the creeps, so I couldn't resist.  :-)
> 
> * - a yes/no/unknown tristate type a-la boost::tribool would be nice
>     to have, IMO.  It could be used more clearly in these situations
>     and could supersede auto_boolean.
> 
> Thanks,
> Pedro Alves

Oh, thanks for pointing out.  I had never realized this but it makes sense.  There should be a compiler
warning about it!  Or maybe it would belong to a linter.

I'll look into adding a tristate bool and fixing it.

Simon

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-21 8:44 p.m., Simon Marchi wrote:
> On 2019-08-21 3:38 p.m., Pedro Alves wrote:
>> std::optional<bool> is evil.  :-)
>>
>> E.g. it's very easy to write (or miss converting)
>>
>>  if (is_static)
>>
>> and not realize that that is doing the wrong thing.
>>
>> https://www.boost.org/doc/libs/1_57_0/libs/optional/doc/html/boost_optional/tutorial/a_note_about_optional_bool_.html
>>
>> Not saying to change the code (*), but seeing it gives me the creeps, so I couldn't resist.  :-)
>>
>> * - a yes/no/unknown tristate type a-la boost::tribool would be nice
>>     to have, IMO.  It could be used more clearly in these situations
>>     and could supersede auto_boolean.
>>
>> Thanks,
>> Pedro Alves
> 
> Oh, thanks for pointing out.  I had never realized this but it makes sense.  There should be a compiler
> warning about it!  Or maybe it would belong to a linter.
> 
> I'll look into adding a tristate bool and fixing it.
> 
> Simon
> 

I started looking into it, seeing if I could implement something like boost's tribool,
but then wondered, why don't we use boost directly?

Its license is compatible with the GPL [1], which from what I understand means that it
can be used to build a GPL program.

[1] https://www.gnu.org/licenses/license-list.html#boost

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.  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?

Here's what a patch to remove the gdb::optional<bool> would look like if we used boost:tribool:


From 440b941b9b839943ccabf90c6370450580e92700 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 22 Aug 2019 19:13:13 -0400
Subject: [PATCH] gdb: Use boost:tribool for is_static in
 dw2_debug_names_iterator::next

---
 gdb/dwarf2read.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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);
   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 ())
     {
 	const bool want_static = *m_block_index == STATIC_BLOCK;
-	if (want_static != *is_static)
+	if (want_static != is_static)
 	  goto again;
     }

-- 
2.23.0

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Pedro Alves]

On 8/23/19 12:36 AM, Simon Marchi wrote:
> On 2019-08-21 8:44 p.m., Simon Marchi wrote:
>> On 2019-08-21 3:38 p.m., Pedro Alves wrote:
>>> std::optional<bool> is evil.  :-)
>>>
>>> E.g. it's very easy to write (or miss converting)
>>>
>>>  if (is_static)
>>>
>>> and not realize that that is doing the wrong thing.
>>>
>>> https://www.boost.org/doc/libs/1_57_0/libs/optional/doc/html/boost_optional/tutorial/a_note_about_optional_bool_.html
>>>
>>> Not saying to change the code (*), but seeing it gives me the creeps, so I couldn't resist.  :-)
>>>
>>> * - a yes/no/unknown tristate type a-la boost::tribool would be nice
>>>     to have, IMO.  It could be used more clearly in these situations
>>>     and could supersede auto_boolean.
>>>
>>> Thanks,
>>> Pedro Alves
>>
>> Oh, thanks for pointing out.  I had never realized this but it makes sense.  There should be a compiler
>> warning about it!  Or maybe it would belong to a linter.
>>
>> I'll look into adding a tristate bool and fixing it.
>>
>> Simon
>>
> 
> I started looking into it, seeing if I could implement something like boost's tribool,
> but then wondered, why don't we use boost directly?
> 
> Its license is compatible with the GPL [1], which from what I understand means that it
> can be used to build a GPL program.
> 
> [1] https://www.gnu.org/licenses/license-list.html#boost
> 
> 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.

> 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().

> 
> Here's what a patch to remove the gdb::optional<bool> would look like if we used boost:tribool:


> 
> 
> From 440b941b9b839943ccabf90c6370450580e92700 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Thu, 22 Aug 2019 19:13:13 -0400
> Subject: [PATCH] gdb: Use boost:tribool for is_static in
>  dw2_debug_names_iterator::next
> 
> ---
>  gdb/dwarf2read.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> 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;

("indeterminate" is really a mouthful, hence "unknown".)

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?

Thanks,
Pedro Alves

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

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;
   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

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Pedro Alves]

On 8/23/19 8:33 PM, Simon Marchi wrote:
> On 2019-08-23 11:35 a.m., Pedro Alves wrote:

>> 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?

That's a very good point actually.  I agree and I'm convinced.

Let's shelve the tribool idea until/if we find a better use for 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.

Right, agreed, the fact that std::map/std::unordered_map searching returns pairs
is one of those things I hate the most about C++.

> Here's a patch that does that.  What do you think?

I think I like it!

Thanks,
Pedro Alves

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-23 3:47 p.m., Pedro Alves wrote:
> On 8/23/19 8:33 PM, Simon Marchi wrote:
>> On 2019-08-23 11:35 a.m., Pedro Alves wrote:
> 
>>> 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?
> 
> That's a very good point actually.  I agree and I'm convinced.
> 
> Let's shelve the tribool idea until/if we find a better use for 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.
> 
> Right, agreed, the fact that std::map/std::unordered_map searching returns pairs
> is one of those things I hate the most about C++.
> 
>> Here's a patch that does that.  What do you think?
> 
> I think I like it!

Yay, less work for me!  I'll run it through the buildbot and push it if it's all good.

Thanks,

Simon

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Pedro Alves]

On 8/23/19 8:52 PM, Simon Marchi wrote:
> On 2019-08-23 3:47 p.m., Pedro Alves wrote:
>> On 8/23/19 8:33 PM, Simon Marchi wrote:
>>> On 2019-08-23 11:35 a.m., Pedro Alves wrote:
>>
>>>> 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?
>>
>> That's a very good point actually.  I agree and I'm convinced.
>>
>> Let's shelve the tribool idea until/if we find a better use for 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.
>>
>> Right, agreed, the fact that std::map/std::unordered_map searching returns pairs
>> is one of those things I hate the most about C++.
>>
>>> Here's a patch that does that.  What do you think?
>>
>> I think I like it!
> 
> Yay, less work for me!  I'll run it through the buildbot and push it if it's all good.

Sorry for the delayed nit, but the first time I didn't really pay attention
the the enumerator names you were using.  Reading back, I notice that you
used "dynamic".  I don't know what that means here?  Don't you mean
"external", as opposed to static?  Like, wouldn't this be more to the point?

  enum {
    symbol_linkage_unknown,
    symbol_linkage_static,
    symbol_linkage_extern,
  } symbol_linkage = symbol_linkage_unknown;

Thanks,
Pedro Alves

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-23 4:23 p.m., Pedro Alves wrote:
> Sorry for the delayed nit, but the first time I didn't really pay attention
> the the enumerator names you were using.  Reading back, I notice that you
> used "dynamic".  I don't know what that means here?  Don't you mean
> "external", as opposed to static?  Like, wouldn't this be more to the point?
> 
>   enum {
>     symbol_linkage_unknown,
>     symbol_linkage_static,
>     symbol_linkage_extern,
>   } symbol_linkage = symbol_linkage_unknown;

Oh yes of course, I was confused static/extern with static/dynamic.  I'll use that, thanks.

Simon

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-23 6:23 p.m., Simon Marchi wrote:
> Oh yes of course, I was confused static/extern with static/dynamic.  I'll use that, thanks.

Arrrg, "I have confused".

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Ruslan Kabatsayev]

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

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-24 07:22, Ruslan Kabatsayev wrote:
> 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).

Good point, I'll try this for the new version.

Simon

Re: [PATCH] Remove some variables in favor of using gdb::optional

[Simon Marchi]

On 2019-08-24 7:55 p.m., Simon Marchi wrote:
> On 2019-08-24 07:22, Ruslan Kabatsayev wrote:
>> 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).
> 
> Good point, I'll try this for the new version.
> 
> Simon

This is what I ended up pushing, thanks to both of you for your comments.

From beadd3e84ed8e652015f07eb4734a6d3b17e79cb Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Sun, 25 Aug 2019 18:09:47 -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/ChangeLog    |  6 ++++++
 gdb/dwarf2read.c | 15 ++++++++++-----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index cbb83347f1e6..5f64ca6d4a94 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2019-08-25  Simon Marchi  <simon.marchi@efficios.com>
+
+	* dwarf2read.c (dw2_debug_names_iterator::next): Use enum to
+	represent whether the symbol is static, dynamic, or we don't
+	know.
+
 2019-08-25  Yoshinori Sato <ysato@users.sourceforge.jp>

         * gdb/rx-tdep.c (rx_register_names): New.
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index de9755f6ce30..a0b989fd0c2e 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 class symbol_linkage {
+    unknown,
+    static_,
+    extern_,
+  } symbol_linkage = symbol_linkage::unknown;
   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_linkage = symbol_linkage::static_;
 	  break;
 	case DW_IDX_GNU_external:
 	  if (!m_map.augmentation_is_gdb)
 	    break;
-	  is_static = false;
+	  symbol_linkage = symbol_linkage::extern_;
 	  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_linkage != symbol_linkage::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_linkage == symbol_linkage::static_;
+	if (want_static != symbol_is_static)
 	  goto again;
     }

-- 
2.23.0