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

[Simon Marchi <simon.marchi@polymtl.ca>]

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