From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ALhkNtt1KWXk2SwAWB0awg (envelope-from ) for ; Fri, 13 Oct 2023 12:52:43 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; secure) header.d=lancelotsix.com header.i=@lancelotsix.com header.a=rsa-sha256 header.s=2021 header.b=Hgj/DdSG; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id D03D31E0C1; Fri, 13 Oct 2023 12:52:43 -0400 (EDT) Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id C0E4D1E091 for ; Fri, 13 Oct 2023 12:52:41 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 42F3E385770D for ; Fri, 13 Oct 2023 16:52:41 +0000 (GMT) Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 41842385770D for ; Fri, 13 Oct 2023 16:52:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 41842385770D Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from octopus (cust120-dsl54.idnet.net [212.69.54.120]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 72F44864F6; Fri, 13 Oct 2023 16:52:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=lancelotsix.com; s=2021; t=1697215948; bh=LdOHhlMc146+wyY3UaEGJSDeeDDeIzn717Q2S0FCyeU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Hgj/DdSGQfSfvoz66MTkaro044OLRa1oYH5xiUADO/s1ciutzXqdI6lSZWsgXBe2h mZwWKlA7QLwYZ6BUDjLYWNlvi+BtUD6gL6CYElWnk7RJKzk4ysA0nhJAT46jofMaiX Z/cJd6HldXAwFvUdMFob2UsLBn1zjukESIlk8l+hejuUtdFaTEjFuLtsw/xfj48nkU eEXoEnOFQohT3HPesSmhu4Bfh86qO0oaEb5RfOeidKhpcHNmBBes6lTb/r0MIfgGbR TD9jBPsHfxEAGwhJqVPsW4YeJQL9EuPb+e3pHRD4X9ZIWN6yAci9hUboiPMFlWj4xd iyna2HhGR1rRQ== Date: Fri, 13 Oct 2023 17:52:23 +0100 From: Lancelot SIX To: Tom de Vries Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight Message-ID: <20231013165223.ahgvhgfbcod2fsz6@octopus> References: <20231013121953.25917-1-tdevries@suse.de> <20231013121953.25917-3-tdevries@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231013121953.25917-3-tdevries@suse.de> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.6.2 (lndn.lancelotsix.com [0.0.0.0]); Fri, 13 Oct 2023 16:52:28 +0000 (UTC) X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Hi Tom On Fri, Oct 13, 2023 at 02:19:51PM +0200, Tom de Vries wrote: > Function source_cache::ensure contains some code using the GNU > source-highlight library. > > The code is a sizable chunk of the function, and contains conditional > compilation in a slightly convoluted way: > ... > if (!already_styled) > #endif /* HAVE_SOURCE_HIGHLIGHT */ > { > ... > > Fix this by factoring out the code into new function try_source_highlight, > such that: > - source_cache::ensure is easier to read, and > - the conditional compilation is at the level of the function body. > > Tested on x86_64-linux. > --- > gdb/source-cache.c | 99 +++++++++++++++++++++++++++------------------- > 1 file changed, 59 insertions(+), 40 deletions(-) > > diff --git a/gdb/source-cache.c b/gdb/source-cache.c > index 7ef54411c69..9757721e932 100644 > --- a/gdb/source-cache.c > +++ b/gdb/source-cache.c > @@ -191,6 +191,63 @@ get_language_name (enum language lang) > > #endif /* HAVE_SOURCE_HIGHLIGHT */ > > +/* Try to highlight CONTENTS from file FULLNAME in language LANG using > + the GNU source-higlight library. Return true if highlighting > + succeeded. */ > + > +static bool > +try_source_highlight (std::string &contents ATTRIBUTE_UNUSED, > + enum language lang ATTRIBUTE_UNUSED, > + const std::string &fullname ATTRIBUTE_UNUSED) > +{ > +#ifdef HAVE_SOURCE_HIGHLIGHT > + if (!use_gnu_source_highlight) > + return false; > + > + const char *lang_name = get_language_name (lang); > + if (lang_name == nullptr) > + return false; > + > + /* The global source highlight object, or null if one was > + never constructed. This is stored here rather than in > + the class so that we don't need to include anything or do > + conditional compilation in source-cache.h. */ > + static srchilite::SourceHighlight *highlighter; Even if this is pre-existing code, I think I would be tempted to get the initialization of HIGHLIGHTER outside of the TRY block (new can throw, we might not want to hide it). Using an immediately invoked lambda does the trick and works well with static variables: static srchilite::SourceHighlight *highlighter = []() { srchilite::SourceHighlight * h = new srchilite::SourceHighlight ("esc.outlang"); h->setStyleFile ("esc.style"); return h; } (); Best, Lancelot. > + > + bool styled = false; > + try > + { > + if (highlighter == nullptr) > + { > + highlighter = new srchilite::SourceHighlight ("esc.outlang"); > + highlighter->setStyleFile ("esc.style"); > + } > + > + std::istringstream input (contents); > + std::ostringstream output; > + highlighter->highlight (input, output, lang_name, fullname); > +#if __cplusplus >= 202002L > + contents = std::move (output).str(); > +#else > + contents = output.str (); > +#endif > + styled = true; > + } > + catch (...) > + { > + /* Source Highlight will throw an exception if > + highlighting fails. One possible reason it can fail > + is if the language is unknown -- which matters to gdb > + because Rust support wasn't added until after 3.1.8. > + Ignore exceptions here. */ > + } > + > + return styled; > +#else > + return false; > +#endif /* HAVE_SOURCE_HIGHLIGHT */ > +} > + > /* See source-cache.h. */ > > bool > @@ -230,48 +287,10 @@ source_cache::ensure (struct symtab *s) > > if (source_styling && gdb_stdout->can_emit_style_escape ()) > { > -#ifdef HAVE_SOURCE_HIGHLIGHT > - bool already_styled = false; > - const char *lang_name = get_language_name (s->language ()); > - if (lang_name != nullptr && use_gnu_source_highlight) > - { > - /* The global source highlight object, or null if one was > - never constructed. This is stored here rather than in > - the class so that we don't need to include anything or do > - conditional compilation in source-cache.h. */ > - static srchilite::SourceHighlight *highlighter; > - > - try > - { > - if (highlighter == nullptr) > - { > - highlighter = new srchilite::SourceHighlight ("esc.outlang"); > - highlighter->setStyleFile ("esc.style"); > - } > - > - std::istringstream input (contents); > - std::ostringstream output; > - highlighter->highlight (input, output, lang_name, fullname); > -#if __cplusplus >= 202002L > - contents = std::move (output).str(); > -#else > - contents = output.str (); > -#endif > - already_styled = true; > - } > - catch (...) > - { > - /* Source Highlight will throw an exception if > - highlighting fails. One possible reason it can fail > - is if the language is unknown -- which matters to gdb > - because Rust support wasn't added until after 3.1.8. > - Ignore exceptions here and fall back to > - un-highlighted text. */ > - } > - } > + bool already_styled > + = try_source_highlight (contents, s->language (), fullname); > > if (!already_styled) > -#endif /* HAVE_SOURCE_HIGHLIGHT */ > { > gdb::optional ext_contents; > ext_contents = ext_lang_colorize (fullname, contents); > -- > 2.35.3 >