From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 007183858D38 for ; Mon, 16 Oct 2023 07:07:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 007183858D38 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 007183858D38 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:67c:2178:6::1d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697440026; cv=none; b=UHyILH9U2YcUa3zW3rdir4Ec7FuyX1qno3XYXfzJw7/XH7vnIl/CSO3mKQWGTpaf9o0nfLtcTxJctT2TgzZZcw7N1P8c/L9W2OsE7ofG6pWU227CkS4Hi98M9GLTWdfNnhbDwxKnHN/Grk71OKXrE5DBLkas64eQqcAzDEOrUos= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697440026; c=relaxed/simple; bh=OpHdMqsMCw80gzDCEoDCHdA/M9wHt62YzdBvWFchUpc=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=Ea1CxyNan61vI2fYXNGLWqAr/MVzcG6oHW5o4YCxpQB8prdjXlhLJulFQ4UO45L9DK8p8xwqgIiWPjAZ7u/Xpfi3tBzdWbKuykcJ5NYpywK0QF9lqYwiaTjQ2zCO6MHzraLCQAbiMGj3z0WsvHGIACan4HXUE6plmicY7+iDQxo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id AB5501FE2A; Mon, 16 Oct 2023 07:07:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1697440021; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=nZPmtcGKoTs9RXwfsphnZyK0p/DnCcm2ttghSYjpF+Q=; b=STTtowYQf0d7dGLMxXHP8fe9R8tDRQbmSgcJV2DmitFx4lqK/cKoaGDoctq96SzTfuvovK tedDCzu6F18xFL5eo9edcywrNCBZEUlAXdjzLvxwdR81EOrsZEsmz6gyUQ/OhEWIItYZXd FJHKM94Nkin76dYrrRz0+f8aIHnVF6Q= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1697440021; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=nZPmtcGKoTs9RXwfsphnZyK0p/DnCcm2ttghSYjpF+Q=; b=ACniExxl1qDRjsY6CyOgLe3MSVKn50eXyKADAcjlrJhMScBlE8qgfgTbbQj3KQAJBOWY3p P9OnbWRxF4t05GDg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 96B97138EF; Mon, 16 Oct 2023 07:07:01 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id Z9shFhXhLGXCVQAAMHmgww (envelope-from ); Mon, 16 Oct 2023 07:07:01 +0000 Message-ID: <524b62b4-bf07-4ed5-b8a2-2ad8dac11043@suse.de> Date: Mon, 16 Oct 2023 09:07:28 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/4] [gdb/cli] Factor out try_source_highlight Content-Language: en-US To: Lancelot SIX Cc: gdb-patches@sourceware.org References: <20231013121953.25917-1-tdevries@suse.de> <20231013121953.25917-3-tdevries@suse.de> <20231013165223.ahgvhgfbcod2fsz6@octopus> From: Tom de Vries In-Reply-To: <20231013165223.ahgvhgfbcod2fsz6@octopus> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Authentication-Results: smtp-out2.suse.de; none X-Spam-Level: X-Spam-Score: -1.09 X-Spamd-Result: default: False [-1.09 / 50.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; XM_UA_NO_VERSION(0.01)[]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; BAYES_HAM(-3.00)[100.00%]; MIME_GOOD(-0.10)[text/plain]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; NEURAL_HAM_SHORT(-1.00)[-1.000]; RCPT_COUNT_TWO(0.00)[2]; NEURAL_SPAM_LONG(3.00)[1.000]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[]; MID_RHS_MATCH_FROM(0.00)[] 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 List-Id: On 10/13/23 18:52, Lancelot SIX wrote: > 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). > Hi, The pre-existing code is a result of commit f64e2f40454 ("[gdb] Catch exception when constructing the highlighter"), which does the opposite of what you propose here. And I think the rationale given there still holds. I suppose if we're interested in not hiding that new throws, you could catch the exceptions of interest and handle them appropriately, by say issuing a warning or rethrowing. I will leave this as is for now. Thanks, - Tom > 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 >>