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

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

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