From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19884 invoked by alias); 6 Feb 2008 02:40:01 -0000 Received: (qmail 19876 invoked by uid 22791); 6 Feb 2008 02:40:00 -0000 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 06 Feb 2008 02:39:44 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 225282AA020 for ; Tue, 5 Feb 2008 21:39:42 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id zVf2B-ORgK8W for ; Tue, 5 Feb 2008 21:39:42 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 7EB872A9FAA for ; Tue, 5 Feb 2008 21:39:41 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id A3B3DE7ACB; Tue, 5 Feb 2008 17:57:03 -0800 (PST) Date: Wed, 06 Feb 2008 02:40:00 -0000 From: Joel Brobecker To: gdb-patches@sourceware.org Subject: [RFA/commit/Ada] Replace home-made string_vector struct with VEC Message-ID: <20080206015703.GA16367@adacore.com> References: <20071228122825.GC24450@adacore.com> <20080129172800.GA3773@caradoc.them.org> <20080204231510.GI21614@adacore.com> <20080204232951.GA31411@caradoc.them.org> <20080205223151.GA3727@adacore.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="zhXaljGHf11kAtnf" Content-Disposition: inline In-Reply-To: <20080205223151.GA3727@adacore.com> User-Agent: Mutt/1.4.2.2i 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 X-SW-Source: 2008-02/txt/msg00118.txt.bz2 --zhXaljGHf11kAtnf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-length: 1834 Hi Daniel, > I've committed the patch as is, and will do the VEC replacement as > a separate patch. I gave your suggestion a try, and here is what I got. I like using the VEC as I feel that it will be more robust in the long run than my local home-made string_vector (even if the code that I wrote was pretty simple to start with). Overall, it doesn't looks like much less code, but that's mostly because of the copy that I have to make, I think. While working on this change, it made me wonder about a few things: 1. Would it make sense to have standard VECs predefined somewhere? For instance, I wonder how often a VEC of strings will be useful in GDB... 2. Would it make sense to expand the VEC API to have a method that will return a copy of the underlying array? After some thoughts, and a look at the current usage, it seemed to me that it didn't make much sense. I didn't see any other places where we need to make that copy, and making that copy is pretty simple. What do you think of the patch? In hindsight, I think it has more merit than I thought. So I think I'll commit it unless you see some flaws in it. 2007-02-05 Joel Brobecker * ada-lang.c: #include "vec.h". (struct string_vector, new_string_vector, string_vector_append): Delete. (char_ptr): New typedef. (DEF_VEC_P (char_ptr)): New VEC type. (symbol_completion_add): Update profile to take the new VEC type instead of the old string_vector structure. Update code accordingly. (ada_make_symbol_completion_list): Use the new VEC type instead of the old string_vector structure, and update the code accordingly. * Makefile.in (ada-lang.o): Add dependency on vec.h. Tested on x86-linux. No regression. -- Joel --zhXaljGHf11kAtnf Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="complete-VEC.diff" Content-length: 7512 Index: ada-lang.c =================================================================== --- ada-lang.c (revision 165) +++ ada-lang.c (revision 166) @@ -55,6 +55,7 @@ #include "valprint.h" #include "source.h" #include "observer.h" +#include "vec.h" /* Define whether or not the C operator '/' truncates towards zero for differently signed operands (truncation direction is undefined in C). @@ -64,25 +65,6 @@ #define TRUNCATION_TOWARDS_ZERO ((-5 / 2) == -2) #endif -/* A structure that contains a vector of strings. - The main purpose of this type is to group the vector and its - associated parameters in one structure. This makes it easier - to handle and pass around. - - brobecker/2008-02-04: GDB does provide a generic VEC which should be - preferable. But we are using the string_vector structure in the context - of symbol completion, and the current infrastructure is such that it's - more convenient to use the string vector for now. It would become - advantageous to switch to VECs if the rest of the completion-related - code switches to VECs as well. */ - -struct string_vector -{ - char **array; /* The vector itself. */ - int index; /* Index of the next available element in the array. */ - size_t size; /* The number of entries allocated in the array. */ -}; - static void extract_string (CORE_ADDR addr, char *buf); static void modify_general_field (char *, LONGEST, int, int); @@ -347,34 +329,6 @@ static struct obstack symbol_list_obstac /* Utilities */ -/* Create a new empty string_vector struct with an initial size of - INITIAL_SIZE. */ - -static struct string_vector -new_string_vector (int initial_size) -{ - struct string_vector result; - - result.array = (char **) xmalloc ((initial_size + 1) * sizeof (char *)); - result.index = 0; - result.size = initial_size; - - return result; -} - -/* Add STR at the end of the given string vector SV. If SV is already - full, its size is automatically increased (doubled). */ - -static void -string_vector_append (struct string_vector *sv, char *str) -{ - if (sv->index >= sv->size) - GROW_VECT (sv->array, sv->size, sv->size * 2); - - sv->array[sv->index] = str; - sv->index++; -} - /* Given DECODED_NAME a string holding a symbol name in its decoded form (ie using the Ada dotted notation), returns its unqualified name. */ @@ -5525,6 +5479,9 @@ symbol_completion_match (const char *sym return sym_name; } +typedef char *char_ptr; +DEF_VEC_P (char_ptr); + /* A companion function to ada_make_symbol_completion_list(). Check if SYM_NAME represents a symbol which name would be suitable to complete TEXT (TEXT_LEN is the length of TEXT), in which case @@ -5541,7 +5498,7 @@ symbol_completion_match (const char *sym encoded). */ static void -symbol_completion_add (struct string_vector *sv, +symbol_completion_add (VEC(char_ptr) *sv, const char *sym_name, const char *text, int text_len, const char *orig_text, const char *word, @@ -5577,7 +5534,7 @@ symbol_completion_add (struct string_vec strcat (completion, match); } - string_vector_append (sv, completion); + VEC_safe_push (char_ptr, sv, completion); } /* Return a list of possible symbol names completing TEXT0. The list @@ -5591,7 +5548,7 @@ ada_make_symbol_completion_list (char *t int text_len; int wild_match; int encoded; - struct string_vector result = new_string_vector (128); + VEC(char_ptr) *completions = VEC_alloc (char_ptr, 128); struct symbol *sym; struct symtab *s; struct partial_symtab *ps; @@ -5640,7 +5597,7 @@ ada_make_symbol_completion_list (char *t + ps->n_global_syms); psym++) { QUIT; - symbol_completion_add (&result, SYMBOL_LINKAGE_NAME (*psym), + symbol_completion_add (completions, SYMBOL_LINKAGE_NAME (*psym), text, text_len, text0, word, wild_match, encoded); } @@ -5650,7 +5607,7 @@ ada_make_symbol_completion_list (char *t + ps->n_static_syms); psym++) { QUIT; - symbol_completion_add (&result, SYMBOL_LINKAGE_NAME (*psym), + symbol_completion_add (completions, SYMBOL_LINKAGE_NAME (*psym), text, text_len, text0, word, wild_match, encoded); } @@ -5664,7 +5621,7 @@ ada_make_symbol_completion_list (char *t ALL_MSYMBOLS (objfile, msymbol) { QUIT; - symbol_completion_add (&result, SYMBOL_LINKAGE_NAME (msymbol), + symbol_completion_add (completions, SYMBOL_LINKAGE_NAME (msymbol), text, text_len, text0, word, wild_match, encoded); } @@ -5678,7 +5635,7 @@ ada_make_symbol_completion_list (char *t ALL_BLOCK_SYMBOLS (b, iter, sym) { - symbol_completion_add (&result, SYMBOL_LINKAGE_NAME (sym), + symbol_completion_add (completions, SYMBOL_LINKAGE_NAME (sym), text, text_len, text0, word, wild_match, encoded); } @@ -5693,7 +5650,7 @@ ada_make_symbol_completion_list (char *t b = BLOCKVECTOR_BLOCK (BLOCKVECTOR (s), GLOBAL_BLOCK); ALL_BLOCK_SYMBOLS (b, iter, sym) { - symbol_completion_add (&result, SYMBOL_LINKAGE_NAME (sym), + symbol_completion_add (completions, SYMBOL_LINKAGE_NAME (sym), text, text_len, text0, word, wild_match, encoded); } @@ -5708,16 +5665,30 @@ ada_make_symbol_completion_list (char *t continue; ALL_BLOCK_SYMBOLS (b, iter, sym) { - symbol_completion_add (&result, SYMBOL_LINKAGE_NAME (sym), + symbol_completion_add (completions, SYMBOL_LINKAGE_NAME (sym), text, text_len, text0, word, wild_match, encoded); } } /* Append the closing NULL entry. */ - string_vector_append (&result, NULL); + VEC_safe_push (char_ptr, completions, NULL); - return (result.array); + /* Make a copy of the COMPLETIONS VEC before we free it, and then + return the copy. It's unfortunate that we have to make a copy + of an array that we're about to destroy, but there is nothing much + we can do about it. Fortunately, it's typically not a very large + array. */ + { + const size_t completions_size = + VEC_length (char_ptr, completions) * sizeof (char *); + char **result = malloc (completions_size); + + memcpy (result, VEC_address (char_ptr, completions), completions_size); + + VEC_free (char_ptr, completions); + return result; + } } /* Breakpoint-related */ Index: Makefile.in =================================================================== --- Makefile.in (revision 165) +++ Makefile.in (revision 166) @@ -1797,7 +1797,7 @@ ada-lang.o: ada-lang.c $(defs_h) $(gdb_s $(gdbcore_h) $(hashtab_h) $(gdb_obstack_h) $(ada_lang_h) \ $(completer_h) $(gdb_stat_h) $(ui_out_h) $(block_h) $(infcall_h) \ $(dictionary_h) $(exceptions_h) $(annotate_h) $(valprint_h) \ - $(source_h) $(observer_h) + $(source_h) $(observer_h) $(vec_h) ada-typeprint.o: ada-typeprint.c $(defs_h) $(gdb_obstack_h) $(bfd_h) \ $(symtab_h) $(gdbtypes_h) $(expression_h) $(value_h) $(gdbcore_h) \ $(target_h) $(command_h) $(gdbcmd_h) $(language_h) $(demangle_h) \ --zhXaljGHf11kAtnf--