diff mbox

Instantiate a single source highlighter

Message ID 20190618194053.7515-1-tromey@adacore.com
State New
Headers show

Commit Message

Tom Tromey June 18, 2019, 7:40 p.m. UTC
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.

gdb/ChangeLog
2019-06-18  Tom Tromey  <tromey@adacore.com>

	* 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(-)

Comments

Tom Tromey June 18, 2019, 8 p.m. UTC | #1
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> +#include <srchilite/parserexception.h>

Oops, I didn't mean to include this hunk.

This came from an attempt to let gdb try to use the rust language from
source highlight (new in git -- not in a release), and then fall back if
it isn't found.  However, that's causing crashes in the library and so
isn't quite ready...

Meanwhile I've removed this include from my patch.

Tom
Pedro Alves June 18, 2019, 8:20 p.m. UTC | #2
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  <tromey@adacore.com>
> 
> 	* 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 <sstream>
>  #include <srchilite/sourcehighlight.h>
>  #include <srchilite/langmap.h>
> +#include <srchilite/parserexception.h>
>  #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<srchilite::SourceHighlight> 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
Philippe Waroquiers June 18, 2019, 9:24 p.m. UTC | #3
On Tue, 2019-06-18 at 21:20 +0100, Pedro Alves wrote:
> > +/* 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?
As highlighter is a global static variable,
when GDB exits, valgrind will consider the memory as reachable
and not a leak.

So, from only a valgrind point of view, there is no reason to have
GDB releasing this memory automatically when exiting.

Philippe
diff mbox

Patch

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 <sstream>
 #include <srchilite/sourcehighlight.h>
 #include <srchilite/langmap.h>
+#include <srchilite/parserexception.h>
 #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
+
 /* 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");
+		}
 
 	      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));