From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81751 invoked by alias); 18 Jun 2019 20:20:07 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 81743 invoked by uid 89); 18 Jun 2019 20:20:07 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=anytime, HX-Spam-Relays-External:209.85.128.66, H*RU:209.85.128.66 X-HELO: mail-wm1-f66.google.com Received: from mail-wm1-f66.google.com (HELO mail-wm1-f66.google.com) (209.85.128.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 18 Jun 2019 20:20:06 +0000 Received: by mail-wm1-f66.google.com with SMTP id z23so4556260wma.4 for ; Tue, 18 Jun 2019 13:20:05 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:56ee:75ff:fe8d:232b? ([2001:8a0:f913:f700:56ee:75ff:fe8d:232b]) by smtp.gmail.com with ESMTPSA id f2sm22800179wrq.48.2019.06.18.13.20.03 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Tue, 18 Jun 2019 13:20:03 -0700 (PDT) Subject: Re: [PATCH] Instantiate a single source highlighter To: Tom Tromey , gdb-patches@sourceware.org References: <20190618194053.7515-1-tromey@adacore.com> From: Pedro Alves Message-ID: Date: Tue, 18 Jun 2019 20:20:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190618194053.7515-1-tromey@adacore.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-06/txt/msg00360.txt.bz2 On 6/18/19 8:40 PM, Tom Tromey wrote: > It occurred to me that there's no reason to make a new source > highlighter each time gdb needs to highlight some source code. > Instead, a single one can be created and then simply reused each time. > > This patch implements this idea. Tested on x86-64 Fedora 29. > Assuming we won't need this from different threads anytime soon, seems fine. Comments below. > gdb/ChangeLog > 2019-06-18 Tom Tromey > > * source-cache.c (highlighter): New global. > (source_cache::get_source_lines): Create a highlighter on demand. > --- > gdb/ChangeLog | 5 +++++ > gdb/source-cache.c | 19 ++++++++++++++++--- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/gdb/source-cache.c b/gdb/source-cache.c > index 2d5b549d971..0fa456116b4 100644 > --- a/gdb/source-cache.c > +++ b/gdb/source-cache.c > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > #endif > > /* The number of source files we'll cache. */ > @@ -43,6 +44,14 @@ > > source_cache g_source_cache; > > +/* 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. */ > +#ifdef HAVE_SOURCE_HIGHLIGHT > +static srchilite::SourceHighlight *highlighter; > +#endif Should it be a unique_ptr so that valgrind doesn't complain about it leaking when gdb exits? > + > /* See source-cache.h. */ > > bool > @@ -209,11 +218,15 @@ source_cache::get_source_lines (struct symtab *s, int first_line, > use-after-free. */ > fullname = symtab_to_fullname (s); > } > - srchilite::SourceHighlight highlighter ("esc.outlang"); > - highlighter.setStyleFile("esc.style"); > + > + if (highlighter == nullptr) > + { > + highlighter = new srchilite::SourceHighlight ("esc.outlang"); > + highlighter->setStyleFile("esc.style"); Preexisting, but missing space before parens. > + } To keep the variable's definition and initialization close by, I'd add a get_highlighter function: static std::unique_ptr highlighter; static srchilite::SourceHighlight * get_highlighter () { if (highlighter == nullptr) { highlighter = new srchilite::SourceHighlight ("esc.outlang"); highlighter->setStyleFile ("esc.style"); } return highlighter.get (); } > > std::ostringstream output; > - highlighter.highlight (input, output, lang_name, fullname); > + highlighter->highlight (input, output, lang_name, fullname); > > source_text result = { fullname, output.str () }; > m_source_map.push_back (std::move (result)); Thanks, Pedro Alves