Message ID | 697c3e3d-4f75-4fb2-685b-a6fa59c7a2a3@polymtl.ca |
---|---|
State | New, archived |
Headers |
Received: (qmail 77608 invoked by alias); 22 Aug 2019 23:36:26 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 77597 invoked by uid 89); 22 Aug 2019 23:36:26 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-14.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy=chances, HTo:U*palves, wondered, 1_57_0 X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 22 Aug 2019 23:36:24 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id x7MNaE2e006181 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 22 Aug 2019 19:36:19 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca x7MNaE2e006181 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=polymtl.ca; s=default; t=1566516980; bh=oOV7sGTAjNBkuhaWIFNBgBnBRsdFAcpji44mrM0mZr8=; h=Subject:From:To:References:Date:In-Reply-To:From; b=LQPiDAPHzyzxLzSBnwFIJzaHBuUaDy5o1SIqMuJcSGj75oDbxawOJ5HqI7vkTjQJP LkwwL2Urc0ixtXqISxEgRuDbAttr3FiknWNYOJz2Nh5Cw29nmsC9BKN9tiZMHcg/fA qW3+ILiMbZwx+/BAkdUgp4wjEHBpJpzHAKvyRzmU= Received: from [172.16.2.176] (wsip-184-188-36-2.sd.sd.cox.net [184.188.36.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id A45271E636; Thu, 22 Aug 2019 19:36:13 -0400 (EDT) Subject: Re: [PATCH] Remove some variables in favor of using gdb::optional From: Simon Marchi <simon.marchi@polymtl.ca> To: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org References: <20190804201023.25628-1-simon.marchi@polymtl.ca> <9b1cdf6d-baae-3a5e-c2ea-fcdf124b7a1b@redhat.com> <65a23d93-bf2e-de1a-9052-f6d75832c2a1@polymtl.ca> Message-ID: <697c3e3d-4f75-4fb2-685b-a6fa59c7a2a3@polymtl.ca> Date: Thu, 22 Aug 2019 19:36:12 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <65a23d93-bf2e-de1a-9052-f6d75832c2a1@polymtl.ca> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes |
Commit Message
Simon Marchi
Aug. 22, 2019, 11:36 p.m. UTC
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(-)
Comments
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
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; }