* [PATCH 0/3] Introduce gdb::function_view & fix completion bug
@ 2017-02-22 14:50 Pedro Alves
2017-02-22 14:50 ` [PATCH 2/3] Use gdb::function_view in iterate_over_symtabs & co Pedro Alves
` (2 more replies)
0 siblings, 3 replies; 27+ messages in thread
From: Pedro Alves @ 2017-02-22 14:50 UTC (permalink / raw)
To: gdb-patches
I noticed that gdb.base/completion.exp fails when run with
--target_board=dwarf4-gdb-index. In order to fix it, I wanted to use
a lambda with iterate_over_symtabs. However, the C-style "function
pointer" + "data pointer" callback style we are still using doesn't
play well with lambdas with captures.
C++11 gave us std::function as type-erased wrapper around arbitrary
callables, however, as I explained at [1] in detail (with references),
std::function is not an ideal fit for transient callbacks. A better
type for that is a non-owning version of std::function. Using modern
C++17 terminology, the natural name for such an object is a "view",
thus "function_view". There are several examples of such a thing in
the interwebs. This series adds such a type to GDB.
This version is more complete than any other I've seen. It supports
function pointers efficiently without hitting undefined behavior, it
supports comparison with NULL like std::function, and it supports
referencing callables that return non-void when the function_view is
expecting void, just like std::function too. (Speaking of which, I've
looked at libstdc++'s std::function to compare APIs, and stolen a few
traits bits from there while at it). Unit tests to cover the whole
API are included too.
[1] - https://sourceware.org/ml/gdb-patches/2017-02/msg00535.html
Regtested on x86_64 Fedora 23, and build tested with GCC 4.8.5 too.
Also pushed to the users/palves/function-view branch on sourceware.org.
Pedro Alves (3):
Introduce gdb::function_view
Use gdb::function_view in iterate_over_symtabs & co
Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index
gdb/Makefile.in | 24 ++-
gdb/ada-lang.c | 107 ++++-------
gdb/common/function-view.h | 320 +++++++++++++++++++++++++++++++
gdb/compile/compile-c-support.c | 16 +-
gdb/dwarf2read.c | 51 +++--
gdb/language.h | 19 +-
gdb/linespec.c | 315 ++++++++++++++----------------
gdb/macrocmd.c | 31 ++-
gdb/macrotab.c | 17 +-
gdb/macrotab.h | 39 ++--
gdb/psymtab.c | 79 ++++----
gdb/symfile-debug.c | 42 ++--
gdb/symfile.c | 13 +-
gdb/symfile.h | 64 +++----
gdb/symmisc.c | 47 ++---
gdb/symtab.c | 329 ++++++++++----------------------
gdb/symtab.h | 33 ++--
gdb/unittests/function-view-selftests.c | 183 ++++++++++++++++++
18 files changed, 1002 insertions(+), 727 deletions(-)
create mode 100644 gdb/common/function-view.h
create mode 100644 gdb/unittests/function-view-selftests.c
--
2.5.5
^ permalink raw reply [flat|nested] 27+ messages in thread* [PATCH 2/3] Use gdb::function_view in iterate_over_symtabs & co 2017-02-22 14:50 [PATCH 0/3] Introduce gdb::function_view & fix completion bug Pedro Alves @ 2017-02-22 14:50 ` Pedro Alves 2017-02-22 22:40 ` Yao Qi 2017-02-22 14:50 ` [PATCH 3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves 2017-02-22 14:50 ` [PATCH 1/3] Introduce gdb::function_view Pedro Alves 2 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-22 14:50 UTC (permalink / raw) To: gdb-patches I wanted to pass a lambda to iterate_over_symtabs (see following patch), so I converted it to function_view, and then the rest is cascaded from that. This gets rid of a bunch of single-use callback functions and corresponding manually managed callback capture types (add_partial_datum, search_symbols_data, etc.) in favor of letting the compiler generate them for us by using lambdas with a capture. In a couple cases, it was more natural to convert the existing function callbacks to function objects (i.e., operator(), e.g., decode_compound_collector). gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * ada-lang.c: Include "common/function-view.h". (ada_iterate_over_symbols): Adjust to use function_view as callback type. (struct add_partial_datum, ada_complete_symbol_matcher): Delete. (ada_make_symbol_completion_list): Use a lambda. (ada_exc_search_name_matches): Delete. (name_matches_regex): New. (ada_add_global_exceptions): Use a lambda and name_matches_regex. * compile/compile-c-support.c: Include "common/function-view.h". (print_one_macro): Change prototype to accept a ui_file pointer. (write_macro_definitions): Use a lambda. * dwarf2read.c: Include "common/function-view.h". (dw2_map_expand_apply, dw2_map_symtabs_matching_filename) (dw2_expand_symtabs_matching): Adjust to use function_view as callback type. * language.h: Include "common/function-view.h". (struct language_defn) <la_iterate_over_symbols>: Adjust to use function_view as callback type. (LA_ITERATE_OVER_SYMBOLS): Remove DATA parameter. * linespec.c: Include "common/function-view.h". (collect_info::add_symbol): New method. (struct symbol_and_data_callback, iterate_inline_only, struct symbol_matcher_data, iterate_name_matcher): Delete. (iterate_over_all_matching_symtabs): Adjust to use function_view as callback type and lambdas. (iterate_over_file_blocks): Adjust to use function_view as callback type. (decode_compound_collector): Now a class with private fields. (decode_compound_collector::release_symbols): New method. (collect_one_symbol): Rename to... (decode_compound_collector::operator()): ... this and adjust. (lookup_prefix_sym): decode_compound_collector construction bits move to decode_compound_collector ctor. Pass the decode_compound_collector object directly as callback. Remove cleanups and use decode_compound_collector::release_symbols instead. (symtab_collector): Now a class with private fields. (symtab_collector::release_symtabs): New method. (add_symtabs_to_list): Rename to... (symtab_collector::operator()): ... this and adjust. (collect_symtabs_from_filename): symtab_collector construction bits move to symtab_collector ctor. Pass the symtab_collector object directly as callback. Remove cleanups and use symtab_collector::release_symtabs instead. (collect_symbols): Delete. (add_matching_symbols_to_info): Use lambdas. * macrocmd.c (print_macro_callback): Delete. (info_macro_command): Use a lambda. (info_macros_command): Pass print_macro_definition as callable directly. (print_one_macro): Remove 'ignore' parameter. (macro_list_command): Adjust. * macrotab.c (macro_for_each_data::fn): Now a function_view. (macro_for_each_data::user_data): Delete field. (foreach_macro): Adjust to call the function_view. (macro_for_each): Adjust to use function_view as callback type. (foreach_macro_in_scope): Adjust to call the function_view. (macro_for_each_in_scope): Adjust to use function_view as callback type. * macrotab.h: Include "common/function-view.h". (macro_callback_fn): Declare a prototype instead of a pointer. Remove "user_data" parameter. (macro_for_each, macro_for_each_in_scope): Adjust to use function_view as callback type. * psymtab.c (partial_map_expand_apply) (psym_map_symtabs_matching_filename, recursively_search_psymtabs): Adjust to use function_view as callback type and to return bool. (psym_expand_symtabs_matching): Adjust to use function_view as callback types. * symfile-debug.c (debug_qf_map_symtabs_matching_filename): Adjust to use function_view as callback type and to return bool. (debug_qf_expand_symtabs_matching): Adjust to use function_view as callback types. * symfile.c (expand_symtabs_matching): Adjust to use function_view as callback types. * symfile.h: Include "common/function-view.h". (expand_symtabs_file_matcher_ftype) (expand_symtabs_symbol_matcher_ftype) (expand_symtabs_exp_notify_ftype): Remove "data" parameter and return bool. (quick_symbol_functions::map_symtabs_matching_filename) (quick_symbol_functions::expand_symtabs_matching): Adjust to use function_view as callback type and return bool. (expand_symtabs_matching): Adjust to use function_view as callback type. (maintenance_expand_name_matcher) (maintenance_expand_file_matcher): Delete. (maintenance_expand_symtabs): Use lambdas. * symtab.c (iterate_over_some_symtabs): Adjust to use function_view as callback types and return bool. (iterate_over_symtabs): Likewise. Use unique_xmalloc_ptr instead of a cleanup. (lookup_symtab_callback): Delete. (lookup_symtab): Use a lambda. (iterate_over_symbols): Adjust to use function_view as callback type. (struct search_symbols_data, search_symbols_file_matches) (search_symbols_name_matches): Delete. (search_symbols): Use a pair of lambdas. (struct add_name_data, add_macro_name, symbol_completion_matcher) (symtab_expansion_callback): Delete. (default_make_symbol_completion_list_break_on_1): Use lambdas. * symtab.h: Include "common/function-view.h". (iterate_over_some_symtabs): Adjust to use function_view as callback type and return bool. (iterate_over_symtabs): Adjust to use function_view as callback type. (symbol_found_callback_ftype): Remove 'data' parameter and return bool. (iterate_over_symbols): Adjust to use function_view as callback type. --- gdb/ada-lang.c | 107 +++++--------- gdb/compile/compile-c-support.c | 16 +- gdb/dwarf2read.c | 51 +++---- gdb/language.h | 19 +-- gdb/linespec.c | 315 ++++++++++++++++++---------------------- gdb/macrocmd.c | 31 ++-- gdb/macrotab.c | 17 +-- gdb/macrotab.h | 39 +++-- gdb/psymtab.c | 79 +++++----- gdb/symfile-debug.c | 42 +++--- gdb/symfile.c | 13 +- gdb/symfile.h | 64 ++++---- gdb/symmisc.c | 47 ++---- gdb/symtab.c | 287 +++++++++++++----------------------- gdb/symtab.h | 33 ++--- 15 files changed, 470 insertions(+), 690 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index 502710a..753409c 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -60,6 +60,7 @@ #include "mi/mi-common.h" #include "arch-utils.h" #include "cli/cli-utils.h" +#include "common/function-view.h" /* Define whether or not the C operator '/' truncates towards zero for differently signed operands (truncation direction is undefined in C). @@ -5850,10 +5851,9 @@ ada_lookup_symbol_list (const char *name0, const struct block *block0, /* Implementation of the la_iterate_over_symbols method. */ static void -ada_iterate_over_symbols (const struct block *block, - const char *name, domain_enum domain, - symbol_found_callback_ftype *callback, - void *data) +ada_iterate_over_symbols + (const struct block *block, const char *name, domain_enum domain, + gdb::function_view<symbol_found_callback_ftype> callback) { int ndefs, i; struct block_symbol *results; @@ -5861,7 +5861,7 @@ ada_iterate_over_symbols (const struct block *block, ndefs = ada_lookup_symbol_list_worker (name, block, domain, &results, 0); for (i = 0; i < ndefs; ++i) { - if (! (*callback) (results[i].symbol, data)) + if (!callback (results[i].symbol)) break; } } @@ -6497,30 +6497,6 @@ symbol_completion_add (VEC(char_ptr) **sv, VEC_safe_push (char_ptr, *sv, completion); } -/* An object of this type is passed as the user_data argument to the - expand_symtabs_matching method. */ -struct add_partial_datum -{ - VEC(char_ptr) **completions; - const char *text; - int text_len; - const char *text0; - const char *word; - int wild_match; - int encoded; -}; - -/* A callback for expand_symtabs_matching. */ - -static int -ada_complete_symbol_matcher (const char *name, void *user_data) -{ - struct add_partial_datum *data = (struct add_partial_datum *) user_data; - - return symbol_completion_match (name, data->text, data->text_len, - data->wild_match, data->encoded) != NULL; -} - /* Return a list of possible symbol names completing TEXT0. WORD is the entire command on which completion is made. */ @@ -6569,19 +6545,16 @@ ada_make_symbol_completion_list (const char *text0, const char *word, } /* First, look at the partial symtab symbols. */ - { - struct add_partial_datum data; - - data.completions = &completions; - data.text = text; - data.text_len = text_len; - data.text0 = text0; - data.word = word; - data.wild_match = wild_match_p; - data.encoded = encoded_p; - expand_symtabs_matching (NULL, ada_complete_symbol_matcher, NULL, - ALL_DOMAIN, &data); - } + expand_symtabs_matching (NULL, + [&] (const char *symname) + { + return symbol_completion_match (symname, + text, text_len, + wild_match_p, + encoded_p); + }, + NULL, + ALL_DOMAIN); /* At this point scan through the misc symbol vectors and add each symbol you find to the list. Eventually we want to ignore @@ -13267,30 +13240,6 @@ sort_remove_dups_ada_exceptions_list (VEC(ada_exc_info) **exceptions, VEC_truncate(ada_exc_info, *exceptions, skip + to_sort_len); } -/* A function intended as the "name_matcher" callback in the struct - quick_symbol_functions' expand_symtabs_matching method. - - SEARCH_NAME is the symbol's search name. - - If USER_DATA is not NULL, it is a pointer to a regext_t object - used to match the symbol (by natural name). Otherwise, when USER_DATA - is null, no filtering is performed, and all symbols are a positive - match. */ - -static int -ada_exc_search_name_matches (const char *search_name, void *user_data) -{ - regex_t *preg = (regex_t *) user_data; - - if (preg == NULL) - return 1; - - /* In Ada, the symbol "search name" is a linkage name, whereas - the regular expression used to do the matching refers to - the natural name. So match against the decoded name. */ - return (regexec (preg, ada_decode (search_name), 0, NULL, 0) == 0); -} - /* Add all exceptions defined by the Ada standard whose name match a regular expression. @@ -13370,6 +13319,15 @@ ada_add_exceptions_from_frame (regex_t *preg, struct frame_info *frame, } } +/* Return true if NAME matches PREG or if PREG is NULL. */ + +static bool +name_matches_regex (const char *name, regex_t *preg) +{ + return (preg == NULL + || regexec (preg, ada_decode (name), 0, NULL, 0) == 0); +} + /* Add all exceptions defined globally whose name name match a regular expression, excluding standard exceptions. @@ -13395,8 +13353,17 @@ ada_add_global_exceptions (regex_t *preg, VEC(ada_exc_info) **exceptions) struct objfile *objfile; struct compunit_symtab *s; - expand_symtabs_matching (NULL, ada_exc_search_name_matches, NULL, - VARIABLES_DOMAIN, preg); + /* In Ada, the symbol "search name" is a linkage name, whereas the + regular expression used to do the matching refers to the natural + name. So match against the decoded name. */ + expand_symtabs_matching (NULL, + [&] (const char *search_name) + { + const char *decoded = ada_decode (search_name); + return name_matches_regex (decoded, preg); + }, + NULL, + VARIABLES_DOMAIN); ALL_COMPUNITS (objfile, s) { @@ -13411,9 +13378,7 @@ ada_add_global_exceptions (regex_t *preg, VEC(ada_exc_info) **exceptions) ALL_BLOCK_SYMBOLS (b, iter, sym) if (ada_is_non_standard_exception_sym (sym) - && (preg == NULL - || regexec (preg, SYMBOL_NATURAL_NAME (sym), - 0, NULL, 0) == 0)) + && name_matches_regex (SYMBOL_NATURAL_NAME (sym), preg)) { struct ada_exc_info info = {SYMBOL_PRINT_NAME (sym), SYMBOL_VALUE_ADDRESS (sym)}; diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c index 7ad0a87..c969b42 100644 --- a/gdb/compile/compile-c-support.c +++ b/gdb/compile/compile-c-support.c @@ -25,6 +25,7 @@ #include "macrotab.h" #include "macroscope.h" #include "regcache.h" +#include "common/function-view.h" /* See compile-internal.h. */ @@ -122,10 +123,8 @@ c_get_compile_context (void) static void print_one_macro (const char *name, const struct macro_definition *macro, struct macro_source_file *source, int line, - void *user_data) + ui_file *file) { - struct ui_file *file = (struct ui_file *) user_data; - /* Don't print command-line defines. They will be supplied another way. */ if (line == 0) @@ -168,7 +167,16 @@ write_macro_definitions (const struct block *block, CORE_ADDR pc, scope = user_macro_scope (); if (scope != NULL && scope->file != NULL && scope->file->table != NULL) - macro_for_each_in_scope (scope->file, scope->line, print_one_macro, file); + { + macro_for_each_in_scope (scope->file, scope->line, + [&] (const char *name, + const macro_definition *macro, + macro_source_file *source, + int line) + { + print_one_macro (name, macro, source, line, file); + }); + } } /* Helper function to construct a header scope for a block of code. diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index 9654fa5..f119750 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -70,6 +70,7 @@ #include "build-id.h" #include "namespace.h" #include "common/gdb_unlinker.h" +#include "common/function-view.h" #include <fcntl.h> #include <sys/types.h> @@ -3490,8 +3491,7 @@ static int dw2_map_expand_apply (struct objfile *objfile, struct dwarf2_per_cu_data *per_cu, const char *name, const char *real_path, - int (*callback) (struct symtab *, void *), - void *data) + gdb::function_view<bool (symtab *)> callback) { struct compunit_symtab *last_made = objfile->compunit_symtabs; @@ -3503,17 +3503,16 @@ dw2_map_expand_apply (struct objfile *objfile, all of them. */ dw2_instantiate_symtab (per_cu); - return iterate_over_some_symtabs (name, real_path, callback, data, - objfile->compunit_symtabs, last_made); + return iterate_over_some_symtabs (name, real_path, objfile->compunit_symtabs, + last_made, callback); } /* Implementation of the map_symtabs_matching_filename method. */ -static int -dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name, - const char *real_path, - int (*callback) (struct symtab *, void *), - void *data) +static bool +dw2_map_symtabs_matching_filename + (struct objfile *objfile, const char *name, const char *real_path, + gdb::function_view<bool (symtab *)> callback) { int i; const char *name_basename = lbasename (name); @@ -3545,8 +3544,8 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name, if (compare_filenames_for_search (this_name, name)) { if (dw2_map_expand_apply (objfile, per_cu, name, real_path, - callback, data)) - return 1; + callback)) + return true; continue; } @@ -3560,8 +3559,8 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name, if (compare_filenames_for_search (this_real_name, name)) { if (dw2_map_expand_apply (objfile, per_cu, name, real_path, - callback, data)) - return 1; + callback)) + return true; continue; } @@ -3573,15 +3572,15 @@ dw2_map_symtabs_matching_filename (struct objfile *objfile, const char *name, && FILENAME_CMP (real_path, this_real_name) == 0) { if (dw2_map_expand_apply (objfile, per_cu, name, real_path, - callback, data)) - return 1; + callback)) + return true; continue; } } } } - return 0; + return false; } /* Struct used to manage iterating over all CUs looking for a symbol. */ @@ -3919,11 +3918,10 @@ dw2_map_matching_symbols (struct objfile *objfile, static void dw2_expand_symtabs_matching (struct objfile *objfile, - expand_symtabs_file_matcher_ftype *file_matcher, - expand_symtabs_symbol_matcher_ftype *symbol_matcher, - expand_symtabs_exp_notify_ftype *expansion_notify, - enum search_domain kind, - void *data) + gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher, + gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher, + gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify, + enum search_domain kind) { int i; offset_type iter; @@ -3979,7 +3977,7 @@ dw2_expand_symtabs_matching { const char *this_real_name; - if (file_matcher (file_data->file_names[j], data, 0)) + if (file_matcher (file_data->file_names[j], false)) { per_cu->v.quick->mark = 1; break; @@ -3989,11 +3987,11 @@ dw2_expand_symtabs_matching files are involved, do a quick comparison of the basenames. */ if (!basenames_may_differ && !file_matcher (lbasename (file_data->file_names[j]), - data, 1)) + true)) continue; this_real_name = dw2_get_real_path (objfile, file_data, j); - if (file_matcher (this_real_name, data, 0)) + if (file_matcher (this_real_name, false)) { per_cu->v.quick->mark = 1; break; @@ -4022,7 +4020,7 @@ dw2_expand_symtabs_matching name = index->constant_pool + MAYBE_SWAP (index->symbol_table[idx]); - if (! (*symbol_matcher) (name, data)) + if (!symbol_matcher (name)) continue; /* The name was matched, now expand corresponding CUs that were @@ -4100,8 +4098,7 @@ dw2_expand_symtabs_matching && symtab_was_null && per_cu->v.quick->compunit_symtab != NULL) { - expansion_notify (per_cu->v.quick->compunit_symtab, - data); + expansion_notify (per_cu->v.quick->compunit_symtab); } } } diff --git a/gdb/language.h b/gdb/language.h index b808c9e..3d21e4e 100644 --- a/gdb/language.h +++ b/gdb/language.h @@ -24,6 +24,7 @@ #define LANGUAGE_H 1 #include "symtab.h" +#include "common/function-view.h" /* Forward decls for prototypes. */ struct value; @@ -372,18 +373,15 @@ struct language_defn The caller is responsible for iterating up through superblocks if desired. - For each one, call CALLBACK with the symbol and the DATA - argument. If CALLBACK returns zero, the iteration ends at that - point. + For each one, call CALLBACK with the symbol. If CALLBACK + returns false, the iteration ends at that point. This field may not be NULL. If the language does not need any special processing here, 'iterate_over_symbols' should be used as the definition. */ - void (*la_iterate_over_symbols) (const struct block *block, - const char *name, - domain_enum domain, - symbol_found_callback_ftype *callback, - void *data); + void (*la_iterate_over_symbols) + (const struct block *block, const char *name, domain_enum domain, + gdb::function_view<symbol_found_callback_ftype> callback); /* Various operations on varobj. */ const struct lang_varobj_ops *la_varobj_ops; @@ -527,9 +525,8 @@ extern enum language set_language (enum language); #define LA_PRINT_ARRAY_INDEX(index_value, stream, options) \ (current_language->la_print_array_index(index_value, stream, options)) -#define LA_ITERATE_OVER_SYMBOLS(BLOCK, NAME, DOMAIN, CALLBACK, DATA) \ - (current_language->la_iterate_over_symbols (BLOCK, NAME, DOMAIN, CALLBACK, \ - DATA)) +#define LA_ITERATE_OVER_SYMBOLS(BLOCK, NAME, DOMAIN, CALLBACK) \ + (current_language->la_iterate_over_symbols (BLOCK, NAME, DOMAIN, CALLBACK)) /* Test a character to decide whether it can be printed in literal form or needs to be printed in another representation. For example, diff --git a/gdb/linespec.c b/gdb/linespec.c index 6308026..5e3732d 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -44,6 +44,7 @@ #include "ada-lang.h" #include "stack.h" #include "location.h" +#include "common/function-view.h" typedef struct symbol *symbolp; DEF_VEC_P (symbolp); @@ -171,8 +172,23 @@ struct collect_info VEC (symbolp) *symbols; VEC (bound_minimal_symbol_d) *minimal_symbols; } result; + + /* Possibly add a symbol to the results. */ + bool add_symbol (symbol *sym); }; +bool +collect_info::add_symbol (symbol *sym) +{ + /* In list mode, add all matching symbols, regardless of class. + This allows the user to type "list a_global_variable". */ + if (SYMBOL_CLASS (sym) == LOC_BLOCK || this->state->list_mode) + VEC_safe_push (symbolp, this->result.symbols, sym); + + /* Continue iterating. */ + return true; +} + /* Token types */ enum ls_token_type @@ -264,10 +280,9 @@ typedef struct ls_parser linespec_parser; /* Prototypes for local functions. */ -static void iterate_over_file_blocks (struct symtab *symtab, - const char *name, domain_enum domain, - symbol_found_callback_ftype *callback, - void *data); +static void iterate_over_file_blocks + (struct symtab *symtab, const char *name, domain_enum domain, + gdb::function_view<symbol_found_callback_ftype> callback); static void initialize_defaults (struct symtab **default_symtab, int *default_line); @@ -916,83 +931,26 @@ maybe_add_address (htab_t set, struct program_space *pspace, CORE_ADDR addr) return 1; } -/* A callback function and the additional data to call it with. */ - -struct symbol_and_data_callback -{ - /* The callback to use. */ - symbol_found_callback_ftype *callback; - - /* Data to be passed to the callback. */ - void *data; -}; - -/* A helper for iterate_over_all_matching_symtabs that is used to - restrict calls to another callback to symbols representing inline - symbols only. */ - -static int -iterate_inline_only (struct symbol *sym, void *d) -{ - if (SYMBOL_INLINED (sym)) - { - struct symbol_and_data_callback *cad - = (struct symbol_and_data_callback *) d; - - return cad->callback (sym, cad->data); - } - return 1; /* Continue iterating. */ -} - -/* Some data for the expand_symtabs_matching callback. */ - -struct symbol_matcher_data -{ - /* The lookup name against which symbol name should be compared. */ - const char *lookup_name; - - /* The routine to be used for comparison. */ - symbol_name_cmp_ftype symbol_name_cmp; -}; - -/* A helper for iterate_over_all_matching_symtabs that is passed as a - callback to the expand_symtabs_matching method. */ - -static int -iterate_name_matcher (const char *name, void *d) -{ - const struct symbol_matcher_data *data - = (const struct symbol_matcher_data *) d; - - if (data->symbol_name_cmp (name, data->lookup_name) == 0) - return 1; /* Expand this symbol's symbol table. */ - return 0; /* Skip this symbol. */ -} - /* A helper that walks over all matching symtabs in all objfiles and calls CALLBACK for each symbol matching NAME. If SEARCH_PSPACE is not NULL, then the search is restricted to just that program - space. If INCLUDE_INLINE is nonzero then symbols representing + space. If INCLUDE_INLINE is true then symbols representing inlined instances of functions will be included in the result. */ static void -iterate_over_all_matching_symtabs (struct linespec_state *state, - const char *name, - const domain_enum domain, - symbol_found_callback_ftype *callback, - void *data, - struct program_space *search_pspace, - int include_inline) +iterate_over_all_matching_symtabs + (struct linespec_state *state, const char *name, const domain_enum domain, + struct program_space *search_pspace, bool include_inline, + gdb::function_view<symbol_found_callback_ftype> callback) { struct objfile *objfile; struct program_space *pspace; - struct symbol_matcher_data matcher_data; - matcher_data.lookup_name = name; - matcher_data.symbol_name_cmp = - state->language->la_get_symbol_name_cmp != NULL - ? state->language->la_get_symbol_name_cmp (name) - : strcmp_iw; + /* The routine to be used for comparison. */ + symbol_name_cmp_ftype symbol_name_cmp + = (state->language->la_get_symbol_name_cmp != NULL + ? state->language->la_get_symbol_name_cmp (name) + : strcmp_iw); ALL_PSPACES (pspace) { @@ -1008,20 +966,24 @@ iterate_over_all_matching_symtabs (struct linespec_state *state, struct compunit_symtab *cu; if (objfile->sf) - objfile->sf->qf->expand_symtabs_matching (objfile, NULL, - iterate_name_matcher, - NULL, ALL_DOMAIN, - &matcher_data); + objfile->sf->qf->expand_symtabs_matching + (objfile, + NULL, + [&] (const char *symbol_name) + { + return symbol_name_cmp (symbol_name, name) == 0; + }, + NULL, + ALL_DOMAIN); ALL_OBJFILE_COMPUNITS (objfile, cu) { struct symtab *symtab = COMPUNIT_FILETABS (cu); - iterate_over_file_blocks (symtab, name, domain, callback, data); + iterate_over_file_blocks (symtab, name, domain, callback); if (include_inline) { - struct symbol_and_data_callback cad = { callback, data }; struct block *block; int i; @@ -1031,7 +993,16 @@ iterate_over_all_matching_symtabs (struct linespec_state *state, { block = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (symtab), i); state->language->la_iterate_over_symbols - (block, name, domain, iterate_inline_only, &cad); + (block, name, + domain, + [&] (symbol *sym) + { + /* Restrict calls to CALLBACK to symbols + representing inline symbols only. */ + if (SYMBOL_INLINED (sym)) + return callback (sym); + return true; + }); } } } @@ -1060,16 +1031,16 @@ get_current_search_block (void) /* Iterate over static and global blocks. */ static void -iterate_over_file_blocks (struct symtab *symtab, - const char *name, domain_enum domain, - symbol_found_callback_ftype *callback, void *data) +iterate_over_file_blocks + (struct symtab *symtab, const char *name, domain_enum domain, + gdb::function_view<symbol_found_callback_ftype> callback) { struct block *block; for (block = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (symtab), STATIC_BLOCK); block != NULL; block = BLOCK_SUPERBLOCK (block)) - LA_ITERATE_OVER_SYMBOLS (block, name, domain, callback, data); + LA_ITERATE_OVER_SYMBOLS (block, name, domain, callback); } /* A helper for find_method. This finds all methods in type T which @@ -2824,60 +2795,70 @@ decode_objc (struct linespec_state *self, linespec_p ls, const char *arg) return values; } -/* An instance of this type is used when collecting prefix symbols for - decode_compound. */ - -struct decode_compound_collector +/* A function object that serves as symbol_found_callback_ftype + callback for iterate_over_symbols. This is used by + lookup_prefix_sym to collect type symbols. */ +class decode_compound_collector { - /* The result vector. */ - VEC (symbolp) *symbols; - - /* A hash table of all symbols we found. We use this to avoid - adding any symbol more than once. */ - htab_t unique_syms; - +public: decode_compound_collector () - : symbols (NULL), - unique_syms (NULL) + : m_symbols (NULL) { + m_unique_syms = htab_create_alloc (1, htab_hash_pointer, + htab_eq_pointer, NULL, + xcalloc, xfree); } ~decode_compound_collector () { - if (unique_syms != NULL) - htab_delete (unique_syms); + if (m_unique_syms != NULL) + htab_delete (m_unique_syms); } -}; -/* A callback for iterate_over_symbols that is used by - lookup_prefix_sym to collect type symbols. */ + /* Releases ownership of the collected symbols and returns them. */ + VEC (symbolp) *release_symbols () + { + VEC (symbolp) *res = m_symbols; + m_symbols = NULL; + return res; + } -static int -collect_one_symbol (struct symbol *sym, void *d) + /* Callable as a symbol_found_callback_ftype callback. */ + bool operator () (symbol *sym); + +private: + /* A hash table of all symbols we found. We use this to avoid + adding any symbol more than once. */ + htab_t m_unique_syms; + + /* The result vector. */ + VEC (symbolp) *m_symbols; +}; + +bool +decode_compound_collector::operator () (symbol *sym) { - struct decode_compound_collector *collector - = (struct decode_compound_collector *) d; void **slot; struct type *t; if (SYMBOL_CLASS (sym) != LOC_TYPEDEF) - return 1; /* Continue iterating. */ + return true; /* Continue iterating. */ t = SYMBOL_TYPE (sym); t = check_typedef (t); if (TYPE_CODE (t) != TYPE_CODE_STRUCT && TYPE_CODE (t) != TYPE_CODE_UNION && TYPE_CODE (t) != TYPE_CODE_NAMESPACE) - return 1; /* Continue iterating. */ + return true; /* Continue iterating. */ - slot = htab_find_slot (collector->unique_syms, sym, INSERT); + slot = htab_find_slot (m_unique_syms, sym, INSERT); if (!*slot) { *slot = sym; - VEC_safe_push (symbolp, collector->symbols, sym); + VEC_safe_push (symbolp, m_symbols, sym); } - return 1; /* Continue iterating. */ + return true; /* Continue iterating. */ } /* Return any symbols corresponding to CLASS_NAME in FILE_SYMTABS. */ @@ -2888,27 +2869,16 @@ lookup_prefix_sym (struct linespec_state *state, VEC (symtab_ptr) *file_symtabs, { int ix; struct symtab *elt; - struct decode_compound_collector collector; - struct cleanup *outer; - struct cleanup *cleanup; - - collector.symbols = NULL; - outer = make_cleanup (VEC_cleanup (symbolp), &collector.symbols); - - collector.unique_syms = htab_create_alloc (1, htab_hash_pointer, - htab_eq_pointer, NULL, - xcalloc, xfree); + decode_compound_collector collector; for (ix = 0; VEC_iterate (symtab_ptr, file_symtabs, ix, elt); ++ix) { if (elt == NULL) { iterate_over_all_matching_symtabs (state, class_name, STRUCT_DOMAIN, - collect_one_symbol, &collector, - NULL, 0); + NULL, false, collector); iterate_over_all_matching_symtabs (state, class_name, VAR_DOMAIN, - collect_one_symbol, &collector, - NULL, 0); + NULL, false, collector); } else { @@ -2916,15 +2886,12 @@ lookup_prefix_sym (struct linespec_state *state, VEC (symtab_ptr) *file_symtabs, been filtered out earlier. */ gdb_assert (!SYMTAB_PSPACE (elt)->executing_startup); set_current_program_space (SYMTAB_PSPACE (elt)); - iterate_over_file_blocks (elt, class_name, STRUCT_DOMAIN, - collect_one_symbol, &collector); - iterate_over_file_blocks (elt, class_name, VAR_DOMAIN, - collect_one_symbol, &collector); + iterate_over_file_blocks (elt, class_name, STRUCT_DOMAIN, collector); + iterate_over_file_blocks (elt, class_name, VAR_DOMAIN, collector); } } - discard_cleanups (outer); - return collector.symbols; + return collector.release_symbols (); } /* A qsort comparison function for symbols. The resulting order does @@ -3130,45 +3097,57 @@ find_method (struct linespec_state *self, VEC (symtab_ptr) *file_symtabs, \f -/* This object is used when collecting all matching symtabs. */ +/* This function object is a callback for iterate_over_symtabs, used + when collecting all matching symtabs. */ -struct symtab_collector +class symtab_collector { - /* The result vector of symtabs. */ - VEC (symtab_ptr) *symtabs; - - /* This is used to ensure the symtabs are unique. */ - htab_t symtab_table; - +public: symtab_collector () - : symtabs (NULL), - symtab_table (NULL) { + m_symtabs = NULL; + m_symtab_table = htab_create (1, htab_hash_pointer, htab_eq_pointer, + NULL); } ~symtab_collector () { - if (symtab_table != NULL) - htab_delete (symtab_table); + if (m_symtab_table != NULL) + htab_delete (m_symtab_table); } -}; -/* Callback for iterate_over_symtabs. */ + /* Callable as a symbol_found_callback_ftype callback. */ + bool operator () (symtab *sym); -static int -add_symtabs_to_list (struct symtab *symtab, void *d) + /* Releases ownership of the collected symtabs and returns them. */ + VEC (symtab_ptr) *release_symtabs () + { + VEC (symtab_ptr) *res = m_symtabs; + m_symtabs = NULL; + return res; + } + +private: + /* The result vector of symtabs. */ + VEC (symtab_ptr) *m_symtabs; + + /* This is used to ensure the symtabs are unique. */ + htab_t m_symtab_table; +}; + +bool +symtab_collector::operator () (struct symtab *symtab) { - struct symtab_collector *data = (struct symtab_collector *) d; void **slot; - slot = htab_find_slot (data->symtab_table, symtab, INSERT); + slot = htab_find_slot (m_symtab_table, symtab, INSERT); if (!*slot) { *slot = symtab; - VEC_safe_push (symtab_ptr, data->symtabs, symtab); + VEC_safe_push (symtab_ptr, m_symtabs, symtab); } - return 0; + return false; } /* Given a file name, return a VEC of all matching symtabs. If @@ -3179,33 +3158,29 @@ static VEC (symtab_ptr) * collect_symtabs_from_filename (const char *file, struct program_space *search_pspace) { - struct symtab_collector collector; - struct cleanup *cleanups; - struct program_space *pspace; - - collector.symtabs = NULL; - collector.symtab_table = htab_create (1, htab_hash_pointer, htab_eq_pointer, - NULL); + symtab_collector collector; /* Find that file's data. */ if (search_pspace == NULL) { + struct program_space *pspace; + ALL_PSPACES (pspace) { if (pspace->executing_startup) continue; set_current_program_space (pspace); - iterate_over_symtabs (file, add_symtabs_to_list, &collector); + iterate_over_symtabs (file, collector); } } else { set_current_program_space (search_pspace); - iterate_over_symtabs (file, add_symtabs_to_list, &collector); + iterate_over_symtabs (file, collector); } - return collector.symtabs; + return collector.release_symtabs (); } /* Return all the symtabs associated to the FILENAME. If SEARCH_PSPACE is @@ -3577,20 +3552,6 @@ linespec_parse_variable (struct linespec_state *self, const char *variable) } \f -/* A callback used to possibly add a symbol to the results. */ - -static int -collect_symbols (struct symbol *sym, void *data) -{ - struct collect_info *info = (struct collect_info *) data; - - /* In list mode, add all matching symbols, regardless of class. - This allows the user to type "list a_global_variable". */ - if (SYMBOL_CLASS (sym) == LOC_BLOCK || info->state->list_mode) - VEC_safe_push (symbolp, info->result.symbols, sym); - return 1; /* Continue iterating. */ -} - /* We've found a minimal symbol MSYMBOL in OBJFILE to associate with our linespec; return the SAL in RESULT. This function should return SALs matching those from find_function_start_sal, otherwise false @@ -3858,8 +3819,8 @@ add_matching_symbols_to_info (const char *name, if (elt == NULL) { iterate_over_all_matching_symtabs (info->state, name, VAR_DOMAIN, - collect_symbols, info, - pspace, 1); + pspace, true, [&] (symbol *sym) + { return info->add_symbol (sym); }); search_minsyms_for_name (info, name, pspace, NULL); } else if (pspace == NULL || pspace == SYMTAB_PSPACE (elt)) @@ -3870,8 +3831,8 @@ add_matching_symbols_to_info (const char *name, been filtered out earlier. */ gdb_assert (!SYMTAB_PSPACE (elt)->executing_startup); set_current_program_space (SYMTAB_PSPACE (elt)); - iterate_over_file_blocks (elt, name, VAR_DOMAIN, - collect_symbols, info); + iterate_over_file_blocks (elt, name, VAR_DOMAIN, [&] (symbol *sym) + { return info->add_symbol (sym); }); /* If no new symbols were found in this iteration and this symtab is in assembler, we might actually be looking for a label for diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c index 69668a2..de28545 100644 --- a/gdb/macrocmd.c +++ b/gdb/macrocmd.c @@ -186,20 +186,6 @@ print_macro_definition (const char *name, fprintf_filtered (gdb_stdout, "=%s\n", d->replacement); } -/* A callback function for usage with macro_for_each and friends. - If USER_DATA is null all macros will be printed. - Otherwise USER_DATA is considered to be a string, printing - only macros who's NAME matches USER_DATA. Other arguments are - routed to print_macro_definition. */ -static void -print_macro_callback (const char *name, const struct macro_definition *macro, - struct macro_source_file *source, int line, - void *user_data) -{ - if (! user_data || strcmp ((const char *) user_data, name) == 0) - print_macro_definition (name, macro, source, line); -} - /* The implementation of the `info macro' command. */ static void info_macro_command (char *args, int from_tty) @@ -248,7 +234,15 @@ info_macro_command (char *args, int from_tty) if (! ms) macro_inform_no_debuginfo (); else if (show_all_macros_named) - macro_for_each (ms->file->table, print_macro_callback, name); + macro_for_each (ms->file->table, + [name] (const char *macro_name, + const macro_definition *macro, + macro_source_file *source, + int line) + { + if (strcmp (name, macro_name) == 0) + print_macro_definition (name, macro, source, line); + }); else { struct macro_definition *d; @@ -296,7 +290,7 @@ info_macros_command (char *args, int from_tty) if (! ms || ! ms->file || ! ms->file->table) macro_inform_no_debuginfo (); else - macro_for_each_in_scope (ms->file, ms->line, print_macro_callback, NULL); + macro_for_each_in_scope (ms->file, ms->line, print_macro_definition); do_cleanups (cleanup_chain); } @@ -465,8 +459,7 @@ macro_undef_command (char *exp, int from_tty) static void print_one_macro (const char *name, const struct macro_definition *macro, - struct macro_source_file *source, int line, - void *ignore) + struct macro_source_file *source, int line) { fprintf_filtered (gdb_stdout, "macro define %s", name); if (macro->kind == macro_function_like) @@ -486,7 +479,7 @@ print_one_macro (const char *name, const struct macro_definition *macro, static void macro_list_command (char *exp, int from_tty) { - macro_for_each (macro_user_macros, print_one_macro, NULL); + macro_for_each (macro_user_macros, print_one_macro); } \f diff --git a/gdb/macrotab.c b/gdb/macrotab.c index 6b3ab6b..1b85c8c 100644 --- a/gdb/macrotab.c +++ b/gdb/macrotab.c @@ -965,8 +965,7 @@ macro_definition_location (struct macro_source_file *source, the FILE and LINE fields. */ struct macro_for_each_data { - macro_callback_fn fn; - void *user_data; + gdb::function_view<macro_callback_fn> fn; struct macro_source_file *file; int line; }; @@ -985,20 +984,18 @@ foreach_macro (splay_tree_node node, void *arg) (struct macro_definition *) node->value); xfree (key_fullname); - (*datum->fn) (key->name, def, key->start_file, key->start_line, - datum->user_data); + datum->fn (key->name, def, key->start_file, key->start_line); return 0; } /* Call FN for every macro in TABLE. */ void -macro_for_each (struct macro_table *table, macro_callback_fn fn, - void *user_data) +macro_for_each (struct macro_table *table, + gdb::function_view<macro_callback_fn> fn) { struct macro_for_each_data datum; datum.fn = fn; - datum.user_data = user_data; datum.file = NULL; datum.line = 0; splay_tree_foreach (table->definitions, foreach_macro, &datum); @@ -1024,20 +1021,18 @@ foreach_macro_in_scope (splay_tree_node node, void *info) && (!key->end_file || compare_locations (key->end_file, key->end_line, datum->file, datum->line) >= 0)) - (*datum->fn) (key->name, def, key->start_file, key->start_line, - datum->user_data); + datum->fn (key->name, def, key->start_file, key->start_line); return 0; } /* Call FN for every macro is visible in SCOPE. */ void macro_for_each_in_scope (struct macro_source_file *file, int line, - macro_callback_fn fn, void *user_data) + gdb::function_view<macro_callback_fn> fn) { struct macro_for_each_data datum; datum.fn = fn; - datum.user_data = user_data; datum.file = file; datum.line = line; splay_tree_foreach (file->table->definitions, diff --git a/gdb/macrotab.h b/gdb/macrotab.h index 0d06dde..16e959a 100644 --- a/gdb/macrotab.h +++ b/gdb/macrotab.h @@ -20,6 +20,8 @@ #ifndef MACROTAB_H #define MACROTAB_H +#include "common/function-view.h" + struct obstack; struct bcache; struct compunit_symtab; @@ -329,28 +331,23 @@ struct macro_source_file *(macro_definition_location const char *name, int *definition_line)); -/* Callback function when walking a macro table. NAME is the name of - the macro, and DEFINITION is the definition. SOURCE is the file at the - start of the include path, and LINE is the line number of the SOURCE file - where the macro was defined. USER_DATA is an arbitrary pointer which is - passed by the caller to macro_for_each or macro_for_each_in_scope. */ -typedef void (*macro_callback_fn) (const char *name, - const struct macro_definition *definition, - struct macro_source_file *source, - int line, - void *user_data); - -/* Call the function FN for each macro in the macro table TABLE. - USER_DATA is passed, untranslated, to FN. */ -void macro_for_each (struct macro_table *table, macro_callback_fn fn, - void *user_data); - -/* Call the function FN for each macro that is visible in a given - scope. The scope is represented by FILE and LINE. USER_DATA is - passed, untranslated, to FN. */ +/* Prototype for a callback callable when walking a macro table. NAME + is the name of the macro, and DEFINITION is the definition. SOURCE + is the file at the start of the include path, and LINE is the line + number of the SOURCE file where the macro was defined. */ +typedef void (macro_callback_fn) (const char *name, + const struct macro_definition *definition, + struct macro_source_file *source, + int line); + +/* Call the callable FN for each macro in the macro table TABLE. */ +void macro_for_each (struct macro_table *table, + gdb::function_view<macro_callback_fn> fn); + +/* Call FN for each macro that is visible in a given scope. The scope + is represented by FILE and LINE. */ void macro_for_each_in_scope (struct macro_source_file *file, int line, - macro_callback_fn fn, - void *user_data); + gdb::function_view<macro_callback_fn> fn); /* Return FILE->filename with possibly prepended compilation directory name. This is raw concatenation without the "set substitute-path" and gdb_realpath diff --git a/gdb/psymtab.c b/gdb/psymtab.c index 6e42bc5..d98a4af 100644 --- a/gdb/psymtab.c +++ b/gdb/psymtab.c @@ -123,13 +123,12 @@ require_partial_symbols (struct objfile *objfile, int verbose) /* Helper function for psym_map_symtabs_matching_filename that expands the symtabs and calls the iterator. */ -static int +static bool partial_map_expand_apply (struct objfile *objfile, const char *name, const char *real_path, struct partial_symtab *pst, - int (*callback) (struct symtab *, void *), - void *data) + gdb::function_view<bool (symtab *)> callback) { struct compunit_symtab *last_made = objfile->compunit_symtabs; @@ -145,20 +144,19 @@ partial_map_expand_apply (struct objfile *objfile, all of them. */ psymtab_to_symtab (objfile, pst); - return iterate_over_some_symtabs (name, real_path, callback, data, - objfile->compunit_symtabs, last_made); + return iterate_over_some_symtabs (name, real_path, objfile->compunit_symtabs, + last_made, callback); } /* Psymtab version of map_symtabs_matching_filename. See its definition in the definition of quick_symbol_functions in symfile.h. */ -static int -psym_map_symtabs_matching_filename (struct objfile *objfile, - const char *name, - const char *real_path, - int (*callback) (struct symtab *, - void *), - void *data) +static bool +psym_map_symtabs_matching_filename + (struct objfile *objfile, + const char *name, + const char *real_path, + gdb::function_view<bool (symtab *)> callback) { struct partial_symtab *pst; const char *name_basename = lbasename (name); @@ -177,8 +175,8 @@ psym_map_symtabs_matching_filename (struct objfile *objfile, if (compare_filenames_for_search (pst->filename, name)) { if (partial_map_expand_apply (objfile, name, real_path, - pst, callback, data)) - return 1; + pst, callback)) + return true; continue; } @@ -191,8 +189,8 @@ psym_map_symtabs_matching_filename (struct objfile *objfile, if (compare_filenames_for_search (psymtab_to_fullname (pst), name)) { if (partial_map_expand_apply (objfile, name, real_path, - pst, callback, data)) - return 1; + pst, callback)) + return true; continue; } @@ -205,14 +203,14 @@ psym_map_symtabs_matching_filename (struct objfile *objfile, if (filename_cmp (psymtab_to_fullname (pst), real_path) == 0) { if (partial_map_expand_apply (objfile, name, real_path, - pst, callback, data)) - return 1; + pst, callback)) + return true; continue; } } } - return 0; + return false; } /* Find which partial symtab contains PC and SECTION starting at psymtab PST. @@ -1291,17 +1289,15 @@ psym_map_matching_symbols (struct objfile *objfile, } } -/* A helper for psym_expand_symtabs_matching that handles - searching included psymtabs. This returns 1 if a symbol is found, - and zero otherwise. It also updates the 'searched_flag' on the +/* A helper for psym_expand_symtabs_matching that handles searching + included psymtabs. This returns true if a symbol is found, and + false otherwise. It also updates the 'searched_flag' on the various psymtabs that it searches. */ -static int -recursively_search_psymtabs (struct partial_symtab *ps, - struct objfile *objfile, - enum search_domain kind, - expand_symtabs_symbol_matcher_ftype *sym_matcher, - void *data) +static bool +recursively_search_psymtabs + (struct partial_symtab *ps, struct objfile *objfile, enum search_domain kind, + gdb::function_view<expand_symtabs_symbol_matcher_ftype> sym_matcher) { struct partial_symbol **psym; struct partial_symbol **bound, **gbound, **sbound; @@ -1323,11 +1319,11 @@ recursively_search_psymtabs (struct partial_symtab *ps, continue; r = recursively_search_psymtabs (ps->dependencies[i], - objfile, kind, sym_matcher, data); + objfile, kind, sym_matcher); if (r != 0) { ps->searched_flag = PST_SEARCHED_AND_FOUND; - return 1; + return true; } } @@ -1365,7 +1361,7 @@ recursively_search_psymtabs (struct partial_symtab *ps, && PSYMBOL_CLASS (*psym) == LOC_BLOCK) || (kind == TYPES_DOMAIN && PSYMBOL_CLASS (*psym) == LOC_TYPEDEF)) - && (*sym_matcher) (SYMBOL_SEARCH_NAME (*psym), data)) + && sym_matcher (SYMBOL_SEARCH_NAME (*psym))) { /* Found a match, so notify our caller. */ result = PST_SEARCHED_AND_FOUND; @@ -1385,11 +1381,10 @@ recursively_search_psymtabs (struct partial_symtab *ps, static void psym_expand_symtabs_matching (struct objfile *objfile, - expand_symtabs_file_matcher_ftype *file_matcher, - expand_symtabs_symbol_matcher_ftype *symbol_matcher, - expand_symtabs_exp_notify_ftype *expansion_notify, - enum search_domain kind, - void *data) + gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher, + gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher, + gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify, + enum search_domain kind) { struct partial_symtab *ps; @@ -1413,31 +1408,31 @@ psym_expand_symtabs_matching if (file_matcher) { - int match; + bool match; if (ps->anonymous) continue; - match = (*file_matcher) (ps->filename, data, 0); + match = file_matcher (ps->filename, false); if (!match) { /* Before we invoke realpath, which can get expensive when many files are involved, do a quick comparison of the basenames. */ if (basenames_may_differ - || (*file_matcher) (lbasename (ps->filename), data, 1)) - match = (*file_matcher) (psymtab_to_fullname (ps), data, 0); + || file_matcher (lbasename (ps->filename), true)) + match = file_matcher (psymtab_to_fullname (ps), false); } if (!match) continue; } - if (recursively_search_psymtabs (ps, objfile, kind, symbol_matcher, data)) + if (recursively_search_psymtabs (ps, objfile, kind, symbol_matcher)) { struct compunit_symtab *symtab = psymtab_to_symtab (objfile, ps); if (expansion_notify != NULL) - expansion_notify (symtab, data); + expansion_notify (symtab); } } } diff --git a/gdb/symfile-debug.c b/gdb/symfile-debug.c index 80039a0..5ca1fa7 100644 --- a/gdb/symfile-debug.c +++ b/gdb/symfile-debug.c @@ -117,28 +117,23 @@ debug_qf_forget_cached_source_info (struct objfile *objfile) debug_data->real_sf->qf->forget_cached_source_info (objfile); } -static int -debug_qf_map_symtabs_matching_filename (struct objfile *objfile, - const char *name, - const char *real_path, - int (*callback) (struct symtab *, - void *), - void *data) +static bool +debug_qf_map_symtabs_matching_filename + (struct objfile *objfile, const char *name, const char *real_path, + gdb::function_view<bool (symtab *)> callback) { const struct debug_sym_fns_data *debug_data = ((const struct debug_sym_fns_data *) objfile_data (objfile, symfile_debug_objfile_data_key)); - int retval; fprintf_filtered (gdb_stdlog, - "qf->map_symtabs_matching_filename (%s, \"%s\", \"%s\", %s, %s)\n", + "qf->map_symtabs_matching_filename (%s, \"%s\", \"%s\", %s)\n", objfile_debug_name (objfile), name, real_path ? real_path : NULL, - host_address_to_string (callback), - host_address_to_string (data)); + host_address_to_string (&callback)); - retval = debug_data->real_sf->qf->map_symtabs_matching_filename - (objfile, name, real_path, callback, data); + bool retval = (debug_data->real_sf->qf->map_symtabs_matching_filename + (objfile, name, real_path, callback)); fprintf_filtered (gdb_stdlog, "qf->map_symtabs_matching_filename (...) = %d\n", @@ -291,29 +286,28 @@ debug_qf_map_matching_symbols (struct objfile *objfile, static void debug_qf_expand_symtabs_matching (struct objfile *objfile, - expand_symtabs_file_matcher_ftype *file_matcher, - expand_symtabs_symbol_matcher_ftype *symbol_matcher, - expand_symtabs_exp_notify_ftype *expansion_notify, - enum search_domain kind, void *data) + gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher, + gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher, + gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify, + enum search_domain kind) { const struct debug_sym_fns_data *debug_data = ((const struct debug_sym_fns_data *) objfile_data (objfile, symfile_debug_objfile_data_key)); fprintf_filtered (gdb_stdlog, - "qf->expand_symtabs_matching (%s, %s, %s, %s, %s, %s)\n", + "qf->expand_symtabs_matching (%s, %s, %s, %s, %s)\n", objfile_debug_name (objfile), - host_address_to_string (file_matcher), - host_address_to_string (symbol_matcher), - host_address_to_string (expansion_notify), - search_domain_name (kind), - host_address_to_string (data)); + host_address_to_string (&file_matcher), + host_address_to_string (&symbol_matcher), + host_address_to_string (&expansion_notify), + search_domain_name (kind)); debug_data->real_sf->qf->expand_symtabs_matching (objfile, file_matcher, symbol_matcher, expansion_notify, - kind, data); + kind); } static struct compunit_symtab * diff --git a/gdb/symfile.c b/gdb/symfile.c index f2528fc..8b79508 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -3856,11 +3856,11 @@ symfile_free_objfile (struct objfile *objfile) See quick_symbol_functions.expand_symtabs_matching for details. */ void -expand_symtabs_matching (expand_symtabs_file_matcher_ftype *file_matcher, - expand_symtabs_symbol_matcher_ftype *symbol_matcher, - expand_symtabs_exp_notify_ftype *expansion_notify, - enum search_domain kind, - void *data) +expand_symtabs_matching + (gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher, + gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher, + gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify, + enum search_domain kind) { struct objfile *objfile; @@ -3869,8 +3869,7 @@ expand_symtabs_matching (expand_symtabs_file_matcher_ftype *file_matcher, if (objfile->sf) objfile->sf->qf->expand_symtabs_matching (objfile, file_matcher, symbol_matcher, - expansion_notify, kind, - data); + expansion_notify, kind); } } diff --git a/gdb/symfile.h b/gdb/symfile.h index 02aee8d..c3e7a31 100644 --- a/gdb/symfile.h +++ b/gdb/symfile.h @@ -26,6 +26,7 @@ #include "symfile-add-flags.h" #include "objfile-flags.h" #include "gdb_bfd.h" +#include "common/function-view.h" /* Opaque declarations. */ struct target_section; @@ -133,20 +134,18 @@ typedef void (symbol_filename_ftype) (const char *filename, /* Callback for quick_symbol_functions->expand_symtabs_matching to match a file name. */ -typedef int (expand_symtabs_file_matcher_ftype) (const char *filename, - void *data, int basenames); +typedef bool (expand_symtabs_file_matcher_ftype) (const char *filename, + bool basenames); /* Callback for quick_symbol_functions->expand_symtabs_matching to match a symbol name. */ -typedef int (expand_symtabs_symbol_matcher_ftype) (const char *name, - void *data); +typedef bool (expand_symtabs_symbol_matcher_ftype) (const char *name); /* Callback for quick_symbol_functions->expand_symtabs_matching to be called after a symtab has been expanded. */ -typedef void (expand_symtabs_exp_notify_ftype) \ - (struct compunit_symtab *symtab, void *data); +typedef void (expand_symtabs_exp_notify_ftype) (compunit_symtab *symtab); /* The "quick" symbol functions exist so that symbol readers can avoiding an initial read of all the symbols. For example, symbol @@ -189,14 +188,11 @@ struct quick_symbol_functions If a match is found, the "partial" symbol table is expanded. Then, this calls iterate_over_some_symtabs (or equivalent) over - all newly-created symbol tables, passing CALLBACK and DATA to it. + all newly-created symbol tables, passing CALLBACK to it. The result of this call is returned. */ - int (*map_symtabs_matching_filename) (struct objfile *objfile, - const char *name, - const char *real_path, - int (*callback) (struct symtab *, - void *), - void *data); + bool (*map_symtabs_matching_filename) + (struct objfile *objfile, const char *name, const char *real_path, + gdb::function_view<bool (symtab *)> callback); /* Check to see if the symbol is defined in a "partial" symbol table of OBJFILE. BLOCK_INDEX should be either GLOBAL_BLOCK or STATIC_BLOCK, @@ -272,30 +268,27 @@ struct quick_symbol_functions /* Expand all symbol tables in OBJFILE matching some criteria. FILE_MATCHER is called for each file in OBJFILE. The file name - and the DATA argument are passed to it. If it returns zero, this - file is skipped. If FILE_MATCHER is NULL such file is not skipped. - If BASENAMES is non-zero the function should consider only base name of - DATA (passed file name is already only the lbasename part). + is passed to it. If the matcher returns false, the file is + skipped. If FILE_MATCHER is NULL the file is not skipped. If + BASENAMES is true the matcher should consider only file base + names (the passed file name is already only the lbasename'd + part). - Otherwise, if KIND does not match this symbol is skipped. + Otherwise, if KIND does not match, this symbol is skipped. - If even KIND matches, then SYMBOL_MATCHER is called for each symbol - defined in the file. The symbol "search" name and DATA are passed - to SYMBOL_MATCHER. + If even KIND matches, SYMBOL_MATCHER is called for each symbol + defined in the file. The symbol "search" name is passed to + SYMBOL_MATCHER. - If SYMBOL_MATCHER returns zero, then this symbol is skipped. + If SYMBOL_MATCHER returns false, then the symbol is skipped. - Otherwise, this symbol's symbol table is expanded. - - DATA is user data that is passed unmodified to the callback - functions. */ + Otherwise, the symbol's symbol table is expanded. */ void (*expand_symtabs_matching) (struct objfile *objfile, - expand_symtabs_file_matcher_ftype *file_matcher, - expand_symtabs_symbol_matcher_ftype *symbol_matcher, - expand_symtabs_exp_notify_ftype *expansion_notify, - enum search_domain kind, - void *data); + gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher, + gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher, + gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify, + enum search_domain kind); /* Return the comp unit from OBJFILE that contains PC and SECTION. Return NULL if there is no such compunit. This @@ -565,10 +558,11 @@ void free_symfile_segment_data (struct symfile_segment_data *data); extern struct cleanup *increment_reading_symtab (void); -void expand_symtabs_matching (expand_symtabs_file_matcher_ftype *, - expand_symtabs_symbol_matcher_ftype *, - expand_symtabs_exp_notify_ftype *, - enum search_domain kind, void *data); +void expand_symtabs_matching + (gdb::function_view<expand_symtabs_file_matcher_ftype> file_matcher, + gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher, + gdb::function_view<expand_symtabs_exp_notify_ftype> expansion_notify, + enum search_domain kind); void map_symbol_filenames (symbol_filename_ftype *fun, void *data, int need_fullname); diff --git a/gdb/symmisc.c b/gdb/symmisc.c index ab50570..32a5331 100644 --- a/gdb/symmisc.c +++ b/gdb/symmisc.c @@ -937,37 +937,6 @@ maintenance_check_symtabs (char *ignore, int from_tty) } } -/* Helper function for maintenance_expand_symtabs. - This is the name_matcher function for expand_symtabs_matching. */ - -static int -maintenance_expand_name_matcher (const char *symname, void *data) -{ - /* Since we're not searching on symbols, just return TRUE. */ - return 1; -} - -/* Helper function for maintenance_expand_symtabs. - This is the file_matcher function for expand_symtabs_matching. */ - -static int -maintenance_expand_file_matcher (const char *filename, void *data, - int basenames) -{ - const char *regexp = (const char *) data; - - QUIT; - - /* KISS: Only apply the regexp to the complete file name. */ - if (basenames) - return 0; - - if (regexp == NULL || re_exec (filename)) - return 1; - - return 0; -} - /* Expand all symbol tables whose name matches an optional regexp. */ static void @@ -1003,8 +972,20 @@ maintenance_expand_symtabs (char *args, int from_tty) if (objfile->sf) { objfile->sf->qf->expand_symtabs_matching - (objfile, maintenance_expand_file_matcher, - maintenance_expand_name_matcher, NULL, ALL_DOMAIN, regexp); + (objfile, + [&] (const char *filename, bool basenames) + { + /* KISS: Only apply the regexp to the complete file name. */ + return (!basenames + && (regexp == NULL || re_exec (filename))); + }, + [] (const char *symname) + { + /* Since we're not searching on symbols, just return true. */ + return true; + }, + NULL, + ALL_DOMAIN); } } diff --git a/gdb/symtab.c b/gdb/symtab.c index 2c141e5..346d5d2 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -382,23 +382,20 @@ compare_glob_filenames_for_search (const char *filename, If NAME is not absolute, then REAL_PATH is NULL If NAME is absolute, then REAL_PATH is the gdb_realpath form of NAME. - The return value, NAME, REAL_PATH, CALLBACK, and DATA - are identical to the `map_symtabs_matching_filename' method of - quick_symbol_functions. + The return value, NAME, REAL_PATH and CALLBACK are identical to the + `map_symtabs_matching_filename' method of quick_symbol_functions. FIRST and AFTER_LAST indicate the range of compunit symtabs to search. Each symtab within the specified compunit symtab is also searched. AFTER_LAST is one past the last compunit symtab to search; NULL means to search until the end of the list. */ -int +bool iterate_over_some_symtabs (const char *name, const char *real_path, - int (*callback) (struct symtab *symtab, - void *data), - void *data, struct compunit_symtab *first, - struct compunit_symtab *after_last) + struct compunit_symtab *after_last, + gdb::function_view<bool (symtab *)> callback) { struct compunit_symtab *cust; struct symtab *s; @@ -410,8 +407,8 @@ iterate_over_some_symtabs (const char *name, { if (compare_filenames_for_search (s->filename, name)) { - if (callback (s, data)) - return 1; + if (callback (s)) + return true; continue; } @@ -423,8 +420,8 @@ iterate_over_some_symtabs (const char *name, if (compare_filenames_for_search (symtab_to_fullname (s), name)) { - if (callback (s, data)) - return 1; + if (callback (s)) + return true; continue; } @@ -438,82 +435,59 @@ iterate_over_some_symtabs (const char *name, gdb_assert (IS_ABSOLUTE_PATH (name)); if (FILENAME_CMP (real_path, fullname) == 0) { - if (callback (s, data)) - return 1; + if (callback (s)) + return true; continue; } } } } - return 0; + return false; } /* Check for a symtab of a specific name; first in symtabs, then in psymtabs. *If* there is no '/' in the name, a match after a '/' in the symtab filename will also work. - Calls CALLBACK with each symtab that is found and with the supplied - DATA. If CALLBACK returns true, the search stops. */ + Calls CALLBACK with each symtab that is found. If CALLBACK returns + true, the search stops. */ void iterate_over_symtabs (const char *name, - int (*callback) (struct symtab *symtab, - void *data), - void *data) + gdb::function_view<bool (symtab *)> callback) { struct objfile *objfile; - char *real_path = NULL; - struct cleanup *cleanups = make_cleanup (null_cleanup, NULL); + gdb::unique_xmalloc_ptr <char> real_path; /* Here we are interested in canonicalizing an absolute path, not absolutizing a relative path. */ if (IS_ABSOLUTE_PATH (name)) { - real_path = gdb_realpath (name); - make_cleanup (xfree, real_path); - gdb_assert (IS_ABSOLUTE_PATH (real_path)); + real_path.reset (gdb_realpath (name)); + gdb_assert (IS_ABSOLUTE_PATH (real_path.get ())); } ALL_OBJFILES (objfile) - { - if (iterate_over_some_symtabs (name, real_path, callback, data, - objfile->compunit_symtabs, NULL)) - { - do_cleanups (cleanups); + { + if (iterate_over_some_symtabs (name, real_path.get (), + objfile->compunit_symtabs, NULL, + callback)) return; - } - } + } /* Same search rules as above apply here, but now we look thru the psymtabs. */ ALL_OBJFILES (objfile) - { - if (objfile->sf - && objfile->sf->qf->map_symtabs_matching_filename (objfile, - name, - real_path, - callback, - data)) - { - do_cleanups (cleanups); + { + if (objfile->sf + && objfile->sf->qf->map_symtabs_matching_filename (objfile, + name, + real_path.get (), + callback)) return; - } - } - - do_cleanups (cleanups); -} - -/* The callback function used by lookup_symtab. */ - -static int -lookup_symtab_callback (struct symtab *symtab, void *data) -{ - struct symtab **result_ptr = (struct symtab **) data; - - *result_ptr = symtab; - return 1; + } } /* A wrapper for iterate_over_symtabs that returns the first matching @@ -524,7 +498,12 @@ lookup_symtab (const char *name) { struct symtab *result = NULL; - iterate_over_symtabs (name, lookup_symtab_callback, &result); + iterate_over_symtabs (name, [&] (symtab *symtab) + { + result = symtab; + return true; + }); + return result; } @@ -2777,18 +2756,17 @@ basic_lookup_transparent_type (const char *name) } /* Iterate over the symbols named NAME, matching DOMAIN, in BLOCK. - - For each symbol that matches, CALLBACK is called. The symbol and - DATA are passed to the callback. - - If CALLBACK returns zero, the iteration ends. Otherwise, the + + For each symbol that matches, CALLBACK is called. The symbol is + passed to the callback. + + If CALLBACK returns false, the iteration ends. Otherwise, the search continues. */ void iterate_over_symbols (const struct block *block, const char *name, const domain_enum domain, - symbol_found_callback_ftype *callback, - void *data) + gdb::function_view<symbol_found_callback_ftype> callback) { struct block_iterator iter; struct symbol *sym; @@ -2798,7 +2776,7 @@ iterate_over_symbols (const struct block *block, const char *name, if (symbol_matches_domain (SYMBOL_LANGUAGE (sym), SYMBOL_DOMAIN (sym), domain)) { - if (!callback (sym, data)) + if (!callback (sym)) return; } } @@ -4282,39 +4260,6 @@ sort_search_symbols_remove_dups (struct symbol_search *found, int nfound, xfree (symbols); } -/* An object of this type is passed as the user_data to the - expand_symtabs_matching method. */ -struct search_symbols_data -{ - int nfiles; - const char **files; - - /* It is true if PREG contains valid data, false otherwise. */ - unsigned preg_p : 1; - regex_t preg; -}; - -/* A callback for expand_symtabs_matching. */ - -static int -search_symbols_file_matches (const char *filename, void *user_data, - int basenames) -{ - struct search_symbols_data *data = (struct search_symbols_data *) user_data; - - return file_matches (filename, data->files, data->nfiles, basenames); -} - -/* A callback for expand_symtabs_matching. */ - -static int -search_symbols_name_matches (const char *symname, void *user_data) -{ - struct search_symbols_data *data = (struct search_symbols_data *) user_data; - - return !data->preg_p || regexec (&data->preg, symname, 0, NULL, 0) == 0; -} - /* Search the symbol table for matches to the regular expression REGEXP, returning the results in *MATCHES. @@ -4359,8 +4304,10 @@ search_symbols (const char *regexp, enum search_domain kind, enum minimal_symbol_type ourtype4; struct symbol_search *found; struct symbol_search *tail; - struct search_symbols_data datum; int nfound; + /* This is true if PREG contains valid data, false otherwise. */ + bool preg_p; + regex_t preg; /* OLD_CHAIN .. RETVAL_CHAIN is always freed, RETVAL_CHAIN .. current CLEANUP_CHAIN is freed only in the case of an error. */ @@ -4375,7 +4322,7 @@ search_symbols (const char *regexp, enum search_domain kind, ourtype4 = types4[kind]; *matches = NULL; - datum.preg_p = 0; + preg_p = false; if (regexp != NULL) { @@ -4414,31 +4361,35 @@ search_symbols (const char *regexp, enum search_domain kind, } } - errcode = regcomp (&datum.preg, regexp, + errcode = regcomp (&preg, regexp, REG_NOSUB | (case_sensitivity == case_sensitive_off ? REG_ICASE : 0)); if (errcode != 0) { - char *err = get_regcomp_error (errcode, &datum.preg); + char *err = get_regcomp_error (errcode, &preg); make_cleanup (xfree, err); error (_("Invalid regexp (%s): %s"), err, regexp); } - datum.preg_p = 1; - make_regfree_cleanup (&datum.preg); + preg_p = true; + make_regfree_cleanup (&preg); } /* Search through the partial symtabs *first* for all symbols matching the regexp. That way we don't have to reproduce all of the machinery below. */ - - datum.nfiles = nfiles; - datum.files = files; - expand_symtabs_matching ((nfiles == 0 - ? NULL - : search_symbols_file_matches), - search_symbols_name_matches, - NULL, kind, &datum); + expand_symtabs_matching ([&] (const char *filename, bool basenames) + { + return file_matches (filename, files, nfiles, + basenames); + }, + [&] (const char *symname) + { + return (!preg_p || regexec (&preg, symname, + 0, NULL, 0) == 0); + }, + NULL, + kind); /* Here, we search through the minimal symbol tables for functions and variables that match, and force their symbols to be read. @@ -4470,8 +4421,8 @@ search_symbols (const char *regexp, enum search_domain kind, || MSYMBOL_TYPE (msymbol) == ourtype3 || MSYMBOL_TYPE (msymbol) == ourtype4) { - if (!datum.preg_p - || regexec (&datum.preg, MSYMBOL_NATURAL_NAME (msymbol), 0, + if (!preg_p + || regexec (&preg, MSYMBOL_NATURAL_NAME (msymbol), 0, NULL, 0) == 0) { /* Note: An important side-effect of these lookup functions @@ -4514,8 +4465,8 @@ search_symbols (const char *regexp, enum search_domain kind, files, nfiles, 1)) && file_matches (symtab_to_fullname (real_symtab), files, nfiles, 0))) - && ((!datum.preg_p - || regexec (&datum.preg, SYMBOL_NATURAL_NAME (sym), 0, + && ((!preg_p + || regexec (&preg, SYMBOL_NATURAL_NAME (sym), 0, NULL, 0) == 0) && ((kind == VARIABLES_DOMAIN && SYMBOL_CLASS (sym) != LOC_TYPEDEF @@ -4572,8 +4523,8 @@ search_symbols (const char *regexp, enum search_domain kind, || MSYMBOL_TYPE (msymbol) == ourtype3 || MSYMBOL_TYPE (msymbol) == ourtype4) { - if (!datum.preg_p - || regexec (&datum.preg, MSYMBOL_NATURAL_NAME (msymbol), 0, + if (!preg_p + || regexec (&preg, MSYMBOL_NATURAL_NAME (msymbol), 0, NULL, 0) == 0) { /* For functions we can do a quick check of whether the @@ -5110,46 +5061,6 @@ completion_list_add_fields (struct symbol *sym, const char *sym_text, } } -/* Type of the user_data argument passed to add_macro_name, - symbol_completion_matcher and symtab_expansion_callback. */ - -struct add_name_data -{ - /* Arguments required by completion_list_add_name. */ - const char *sym_text; - int sym_text_len; - const char *text; - const char *word; - - /* Extra argument required for add_symtab_completions. */ - enum type_code code; -}; - -/* A callback used with macro_for_each and macro_for_each_in_scope. - This adds a macro's name to the current completion list. */ - -static void -add_macro_name (const char *name, const struct macro_definition *ignore, - struct macro_source_file *ignore2, int ignore3, - void *user_data) -{ - struct add_name_data *datum = (struct add_name_data *) user_data; - - completion_list_add_name (name, - datum->sym_text, datum->sym_text_len, - datum->text, datum->word); -} - -/* A callback for expand_symtabs_matching. */ - -static int -symbol_completion_matcher (const char *name, void *user_data) -{ - struct add_name_data *datum = (struct add_name_data *) user_data; - - return compare_symbol_name (name, datum->sym_text, datum->sym_text_len); -} - /* Add matching symbols from SYMTAB to the current completion list. */ static void @@ -5182,21 +5093,6 @@ add_symtab_completions (struct compunit_symtab *cust, } } -/* Callback to add completions to the current list when symbol tables - are expanded during completion list generation. */ - -static void -symtab_expansion_callback (struct compunit_symtab *symtab, - void *user_data) -{ - struct add_name_data *datum = (struct add_name_data *) user_data; - - add_symtab_completions (symtab, - datum->sym_text, datum->sym_text_len, - datum->text, datum->word, - datum->code); -} - static void default_make_symbol_completion_list_break_on_1 (const char *text, const char *word, @@ -5218,7 +5114,6 @@ default_make_symbol_completion_list_break_on_1 (const char *text, const char *sym_text; /* Length of sym_text. */ int sym_text_len; - struct add_name_data datum; struct cleanup *cleanups; /* Now look for the symbol we are supposed to complete on. */ @@ -5292,12 +5187,6 @@ default_make_symbol_completion_list_break_on_1 (const char *text, completion_tracker = new_completion_tracker (); cleanups = make_cleanup_free_completion_tracker (&completion_tracker); - datum.sym_text = sym_text; - datum.sym_text_len = sym_text_len; - datum.text = text; - datum.word = word; - datum.code = code; - /* At this point scan through the misc symbol vectors and add each symbol you find to the list. Eventually we want to ignore anything that isn't a text symbol (everything else will be @@ -5321,13 +5210,22 @@ default_make_symbol_completion_list_break_on_1 (const char *text, add_symtab_completions (cust, sym_text, sym_text_len, text, word, code); - /* Look through the partial symtabs for all symbols which begin - by matching SYM_TEXT. Expand all CUs that you find to the list. - symtab_expansion_callback is called for each expanded symtab, - causing those symtab's completions to be added to the list too. */ - expand_symtabs_matching (NULL, symbol_completion_matcher, - symtab_expansion_callback, ALL_DOMAIN, - &datum); + /* Look through the partial symtabs for all symbols which begin by + matching SYM_TEXT. Expand all CUs that you find to the list. */ + expand_symtabs_matching (NULL, + [&] (const char *name) /* symbol matcher */ + { + return compare_symbol_name (name, + sym_text, + sym_text_len); + }, + [&] (compunit_symtab *symtab) /* expansion notify */ + { + add_symtab_completions (symtab, + sym_text, sym_text_len, + text, word, code); + }, + ALL_DOMAIN); /* Search upwards from currently selected frame (so that we can complete on local vars). Also catch fields of types defined in @@ -5385,6 +5283,17 @@ default_make_symbol_completion_list_break_on_1 (const char *text, { struct macro_scope *scope; + /* This adds a macro's name to the current completion list. */ + auto add_macro_name = [&] (const char *macro_name, + const macro_definition *, + macro_source_file *, + int) + { + completion_list_add_name (macro_name, + sym_text, sym_text_len, + text, word); + }; + /* Add any macros visible in the default scope. Note that this may yield the occasional wrong result, because an expression might be evaluated in a scope other than the default. For @@ -5396,12 +5305,12 @@ default_make_symbol_completion_list_break_on_1 (const char *text, if (scope) { macro_for_each_in_scope (scope->file, scope->line, - add_macro_name, &datum); + add_macro_name); xfree (scope); } /* User-defined macros are always visible. */ - macro_for_each (macro_user_macros, add_macro_name, &datum); + macro_for_each (macro_user_macros, add_macro_name); } do_cleanups (cleanups); diff --git a/gdb/symtab.h b/gdb/symtab.h index 88bdd27..d8c665c 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -24,6 +24,7 @@ #include "gdb_vecs.h" #include "gdbtypes.h" #include "common/enum-flags.h" +#include "common/function-view.h" /* Opaque declarations. */ struct ui_file; @@ -1607,35 +1608,29 @@ int compare_filenames_for_search (const char *filename, int compare_glob_filenames_for_search (const char *filename, const char *search_name); -int iterate_over_some_symtabs (const char *name, - const char *real_path, - int (*callback) (struct symtab *symtab, - void *data), - void *data, - struct compunit_symtab *first, - struct compunit_symtab *after_last); +bool iterate_over_some_symtabs (const char *name, + const char *real_path, + struct compunit_symtab *first, + struct compunit_symtab *after_last, + gdb::function_view<bool (symtab *)> callback); void iterate_over_symtabs (const char *name, - int (*callback) (struct symtab *symtab, - void *data), - void *data); + gdb::function_view<bool (symtab *)> callback); + VEC (CORE_ADDR) *find_pcs_for_symtab_line (struct symtab *symtab, int line, struct linetable_entry **best_entry); -/* Callback for LA_ITERATE_OVER_SYMBOLS. The callback will be called - once per matching symbol SYM, with DATA being the argument of the - same name that was passed to LA_ITERATE_OVER_SYMBOLS. The callback - should return nonzero to indicate that LA_ITERATE_OVER_SYMBOLS - should continue iterating, or zero to indicate that the iteration - should end. */ +/* Prototype for callbacks for LA_ITERATE_OVER_SYMBOLS. The callback + is called once per matching symbol SYM. The callback should return + true to indicate that LA_ITERATE_OVER_SYMBOLS should continue + iterating, or false to indicate that the iteration should end. */ -typedef int (symbol_found_callback_ftype) (struct symbol *sym, void *data); +typedef bool (symbol_found_callback_ftype) (symbol *sym); void iterate_over_symbols (const struct block *block, const char *name, const domain_enum domain, - symbol_found_callback_ftype *callback, - void *data); + gdb::function_view<symbol_found_callback_ftype> callback); /* Storage type used by demangle_for_lookup. demangle_for_lookup either returns a const char * pointer that points to either of the -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/3] Use gdb::function_view in iterate_over_symtabs & co 2017-02-22 14:50 ` [PATCH 2/3] Use gdb::function_view in iterate_over_symtabs & co Pedro Alves @ 2017-02-22 22:40 ` Yao Qi 0 siblings, 0 replies; 27+ messages in thread From: Yao Qi @ 2017-02-22 22:40 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 17-02-22 14:50:32, Pedro Alves wrote: > I wanted to pass a lambda to iterate_over_symtabs (see following > patch), so I converted it to function_view, and then the rest is > cascaded from that. > > This gets rid of a bunch of single-use callback functions and > corresponding manually managed callback capture types > (add_partial_datum, search_symbols_data, etc.) in favor of letting the > compiler generate them for us by using lambdas with a capture. In a > couple cases, it was more natural to convert the existing function > callbacks to function objects (i.e., operator(), e.g., > decode_compound_collector). > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * ada-lang.c: Include "common/function-view.h". > (ada_iterate_over_symbols): Adjust to use function_view as > callback type. > (struct add_partial_datum, ada_complete_symbol_matcher): Delete. > (ada_make_symbol_completion_list): Use a lambda. > (ada_exc_search_name_matches): Delete. > (name_matches_regex): New. > (ada_add_global_exceptions): Use a lambda and name_matches_regex. > * compile/compile-c-support.c: Include "common/function-view.h". > (print_one_macro): Change prototype to accept a ui_file pointer. > (write_macro_definitions): Use a lambda. > * dwarf2read.c: Include "common/function-view.h". > (dw2_map_expand_apply, dw2_map_symtabs_matching_filename) > (dw2_expand_symtabs_matching): Adjust to use function_view as > callback type. > * language.h: Include "common/function-view.h". > (struct language_defn) <la_iterate_over_symbols>: Adjust to use > function_view as callback type. > (LA_ITERATE_OVER_SYMBOLS): Remove DATA parameter. > * linespec.c: Include "common/function-view.h". > (collect_info::add_symbol): New method. > (struct symbol_and_data_callback, iterate_inline_only, struct > symbol_matcher_data, iterate_name_matcher): Delete. > (iterate_over_all_matching_symtabs): Adjust to use function_view > as callback type and lambdas. > (iterate_over_file_blocks): Adjust to use function_view as > callback type. > (decode_compound_collector): Now a class with private fields. > (decode_compound_collector::release_symbols): New method. > (collect_one_symbol): Rename to... > (decode_compound_collector::operator()): ... this and adjust. > (lookup_prefix_sym): decode_compound_collector construction bits > move to decode_compound_collector ctor. Pass the > decode_compound_collector object directly as callback. Remove > cleanups and use decode_compound_collector::release_symbols > instead. > (symtab_collector): Now a class with private fields. > (symtab_collector::release_symtabs): New method. > (add_symtabs_to_list): Rename to... > (symtab_collector::operator()): ... this and adjust. > (collect_symtabs_from_filename): symtab_collector construction > bits move to symtab_collector ctor. Pass the symtab_collector > object directly as callback. Remove cleanups and use > symtab_collector::release_symtabs instead. > (collect_symbols): Delete. > (add_matching_symbols_to_info): Use lambdas. > * macrocmd.c (print_macro_callback): Delete. > (info_macro_command): Use a lambda. > (info_macros_command): Pass print_macro_definition as callable > directly. > (print_one_macro): Remove 'ignore' parameter. > (macro_list_command): Adjust. > * macrotab.c (macro_for_each_data::fn): Now a function_view. > (macro_for_each_data::user_data): Delete field. > (foreach_macro): Adjust to call the function_view. > (macro_for_each): Adjust to use function_view as callback type. > (foreach_macro_in_scope): Adjust to call the function_view. > (macro_for_each_in_scope): Adjust to use function_view as callback > type. > * macrotab.h: Include "common/function-view.h". > (macro_callback_fn): Declare a prototype instead of a pointer. > Remove "user_data" parameter. > (macro_for_each, macro_for_each_in_scope): Adjust to use > function_view as callback type. > * psymtab.c (partial_map_expand_apply) > (psym_map_symtabs_matching_filename, recursively_search_psymtabs): > Adjust to use function_view as callback type and to return bool. > (psym_expand_symtabs_matching): Adjust to use function_view as > callback types. > * symfile-debug.c (debug_qf_map_symtabs_matching_filename): Adjust > to use function_view as callback type and to return bool. > (debug_qf_expand_symtabs_matching): Adjust to use function_view as > callback types. > * symfile.c (expand_symtabs_matching): Adjust to use function_view > as callback types. > * symfile.h: Include "common/function-view.h". > (expand_symtabs_file_matcher_ftype) > (expand_symtabs_symbol_matcher_ftype) > (expand_symtabs_exp_notify_ftype): Remove "data" parameter and > return bool. > (quick_symbol_functions::map_symtabs_matching_filename) > (quick_symbol_functions::expand_symtabs_matching): Adjust to use > function_view as callback type and return bool. > (expand_symtabs_matching): Adjust to use function_view as callback > type. > (maintenance_expand_name_matcher) > (maintenance_expand_file_matcher): Delete. > (maintenance_expand_symtabs): Use lambdas. > * symtab.c (iterate_over_some_symtabs): Adjust to use > function_view as callback types and return bool. > (iterate_over_symtabs): Likewise. Use unique_xmalloc_ptr instead > of a cleanup. > (lookup_symtab_callback): Delete. > (lookup_symtab): Use a lambda. > (iterate_over_symbols): Adjust to use function_view as callback > type. > (struct search_symbols_data, search_symbols_file_matches) > (search_symbols_name_matches): Delete. > (search_symbols): Use a pair of lambdas. > (struct add_name_data, add_macro_name, symbol_completion_matcher) > (symtab_expansion_callback): Delete. > (default_make_symbol_completion_list_break_on_1): Use lambdas. > * symtab.h: Include "common/function-view.h". > (iterate_over_some_symtabs): Adjust to use function_view as > callback type and return bool. > (iterate_over_symtabs): Adjust to use function_view as callback > type. > (symbol_found_callback_ftype): Remove 'data' parameter and return > bool. > (iterate_over_symbols): Adjust to use function_view as callback > type. Patch is good to me. -- Yao ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index 2017-02-22 14:50 [PATCH 0/3] Introduce gdb::function_view & fix completion bug Pedro Alves 2017-02-22 14:50 ` [PATCH 2/3] Use gdb::function_view in iterate_over_symtabs & co Pedro Alves @ 2017-02-22 14:50 ` Pedro Alves 2017-02-23 16:02 ` Yao Qi 2017-02-22 14:50 ` [PATCH 1/3] Introduce gdb::function_view Pedro Alves 2 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-22 14:50 UTC (permalink / raw) To: gdb-patches This patch fixes: -FAIL: gdb.base/completion.exp: tab complete break break.c:ma (timeout) -FAIL: gdb.base/completion.exp: complete break break.c:ma +PASS: gdb.base/completion.exp: tab complete break break.c:ma +PASS: gdb.base/completion.exp: delete breakpoint for tab complete break break.c:ma +PASS: gdb.base/completion.exp: complete break break.c:ma When run with --target_board=dwarf4-gdb-index. The issue here is that make_file_symbol_completion_list_1, used when completing a symbol restricted to a given source file, uses lookup_symtab to look up the symtab with the given name, and search for matching symbols inside. This assumes that there's only one symtab for the given source file. This is an incorrect assumption with (for example) -fdebug-types-section, where we'll have an extra extra symtab containing the types. lookup_symtab finds that symtab, and inside that symtab there are no functions... gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * symtab.c (make_file_symbol_completion_list_1): Iterate over symtabs matching all symtabs with SRCFILE as file name instead of only considering the first hit, with lookup_symtab. --- gdb/symtab.c | 42 ++++++++---------------------------------- 1 file changed, 8 insertions(+), 34 deletions(-) diff --git a/gdb/symtab.c b/gdb/symtab.c index 346d5d2..66fa530 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -5391,10 +5391,6 @@ static VEC (char_ptr) * make_file_symbol_completion_list_1 (const char *text, const char *word, const char *srcfile) { - struct symbol *sym; - struct symtab *s; - struct block *b; - struct block_iterator iter; /* The symbol we are completing on. Points in same buffer as text. */ const char *sym_text; /* Length of sym_text. */ @@ -5445,37 +5441,15 @@ make_file_symbol_completion_list_1 (const char *text, const char *word, sym_text_len = strlen (sym_text); - /* Find the symtab for SRCFILE (this loads it if it was not yet read - in). */ - s = lookup_symtab (srcfile); - if (s == NULL) + /* Go through symtabs for SRCFILE and check the externs and statics + for symbols which match. */ + iterate_over_symtabs (srcfile, [&] (struct symtab *symtab) { - /* Maybe they typed the file with leading directories, while the - symbol tables record only its basename. */ - const char *tail = lbasename (srcfile); - - if (tail > srcfile) - s = lookup_symtab (tail); - } - - /* If we have no symtab for that file, return an empty list. */ - if (s == NULL) - return (return_val); - - /* Go through this symtab and check the externs and statics for - symbols which match. */ - - b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), GLOBAL_BLOCK); - ALL_BLOCK_SYMBOLS (b, iter, sym) - { - COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word); - } - - b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), STATIC_BLOCK); - ALL_BLOCK_SYMBOLS (b, iter, sym) - { - COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word); - } + add_symtab_completions (symtab->compunit_symtab, + sym_text, sym_text_len, + text, word, TYPE_CODE_UNDEF); + return false; + }); return (return_val); } -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index 2017-02-22 14:50 ` [PATCH 3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves @ 2017-02-23 16:02 ` Yao Qi 2017-02-23 17:12 ` Pedro Alves 2017-02-23 17:24 ` [PATCH v2 0/2] " Pedro Alves 0 siblings, 2 replies; 27+ messages in thread From: Yao Qi @ 2017-02-23 16:02 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: Hi Pedro, I don't know much about symbtab, > - /* Find the symtab for SRCFILE (this loads it if it was not yet read > - in). */ > - s = lookup_symtab (srcfile); > - if (s == NULL) > + /* Go through symtabs for SRCFILE and check the externs and statics > + for symbols which match. */ > + iterate_over_symtabs (srcfile, [&] (struct symtab *symtab) > { > - /* Maybe they typed the file with leading directories, while the > - symbol tables record only its basename. */ > - const char *tail = lbasename (srcfile); > - > - if (tail > srcfile) > - s = lookup_symtab (tail); This is removed, do we have something equivalent in iterate_over_symtabs? > - } > - > - /* If we have no symtab for that file, return an empty list. */ > - if (s == NULL) > - return (return_val); > - > - /* Go through this symtab and check the externs and statics for > - symbols which match. */ > - > - b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), GLOBAL_BLOCK); > - ALL_BLOCK_SYMBOLS (b, iter, sym) > - { > - COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word); > - } > - > - b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), STATIC_BLOCK); > - ALL_BLOCK_SYMBOLS (b, iter, sym) > - { > - COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word); > - } > + add_symtab_completions (symtab->compunit_symtab, use SYMTAB_COMPUNIT (s) > + sym_text, sym_text_len, > + text, word, TYPE_CODE_UNDEF); Can you split this part out this patch as a pure refactor? -- Yao (齐尧) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index 2017-02-23 16:02 ` Yao Qi @ 2017-02-23 17:12 ` Pedro Alves 2017-02-23 17:24 ` [PATCH v2 0/2] " Pedro Alves 1 sibling, 0 replies; 27+ messages in thread From: Pedro Alves @ 2017-02-23 17:12 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 02/23/2017 04:02 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > > Hi Pedro, > I don't know much about symbtab, > >> - /* Find the symtab for SRCFILE (this loads it if it was not yet read >> - in). */ >> - s = lookup_symtab (srcfile); >> - if (s == NULL) >> + /* Go through symtabs for SRCFILE and check the externs and statics >> + for symbols which match. */ >> + iterate_over_symtabs (srcfile, [&] (struct symtab *symtab) >> { >> - /* Maybe they typed the file with leading directories, while the >> - symbol tables record only its basename. */ >> - const char *tail = lbasename (srcfile); >> - >> - if (tail > srcfile) >> - s = lookup_symtab (tail); > > This is removed, do we have something equivalent in iterate_over_symtabs? AFAICS, yes, though via other means. iterate_over_some_symtabs matches both the symtab name, and if that fails, tries the symtab's resolved fullname. The path actually fixes a bug related to this, it seems. Before the patch, any random absolute path would match, like: (gdb) b /silly/path/asdfasdfsdafasdfasdfsadfasdfasdfasdfasdfasdf/test.c:<tab> a foo int main() But then actually setting the breakpoint would fail: (gdb) b /silly/path/asdfasdfsdafasdfasdfsadfasdfasdfasdfasdfasdf/test.c:main No source file named /silly/path/asdfasdfsdafasdfasdfsadfasdfasdfasdfasdfasdf/test.c. Make breakpoint pending on future shared library load? (y or [n]) n While after the patch, only full paths that gdb can resolve in find_and_open_source, according to the source lookup rules, match for completion. I.e., in my quick test, completing "/home/pedro/tmp/test.c:", which is the source file's real path, matches for completion, but the silly path above does not. If I move the source file elsewhere and use the "dir" command to point GDB at it, completion now finds it there: (gdb) b /home/pedro/tmp2/test.c:<tab> #nothing (gdb) dir /home/pedro/tmp2/ Source directories searched: /home/pedro/tmp2:$cdir:$cwd (gdb) b /home/pedro/tmp2/test.c: a foo int main() (gdb) b /home/pedro/tmp/dirs/ctor.cc:main Note: breakpoint 1 also set at pc 0x4005ba. So this patch makes completion consistent with the rest of gdb, which seems like a desirable change. I'll try to come up with a test case for this. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 0/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index 2017-02-23 16:02 ` Yao Qi 2017-02-23 17:12 ` Pedro Alves @ 2017-02-23 17:24 ` Pedro Alves 2017-02-23 17:24 ` [PATCH v2 1/2] symtab.c: Small refactor Pedro Alves 2017-02-23 17:24 ` [PATCH v2 2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves 1 sibling, 2 replies; 27+ messages in thread From: Pedro Alves @ 2017-02-23 17:24 UTC (permalink / raw) To: gdb-patches So meanwhile I've pushed in patches #1 and #2. That leaves the bug fixing, now split in two patches. On 02/23/2017 04:02 PM, Yao Qi wrote: >> > + add_symtab_completions (symtab->compunit_symtab, > use SYMTAB_COMPUNIT (s) > >> > + sym_text, sym_text_len, >> > + text, word, TYPE_CODE_UNDEF); > Can you split this part out this patch as a pure refactor? Sure thing. I'll send them as replies to this email. Pedro Alves (2): symtab.c: Small refactor Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index gdb/symtab.c | 42 ++++++++---------------------------------- 1 file changed, 8 insertions(+), 34 deletions(-) -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/2] symtab.c: Small refactor 2017-02-23 17:24 ` [PATCH v2 0/2] " Pedro Alves @ 2017-02-23 17:24 ` Pedro Alves 2017-02-23 21:45 ` Yao Qi 2017-02-23 17:24 ` [PATCH v2 2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves 1 sibling, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-23 17:24 UTC (permalink / raw) To: gdb-patches add_symtab_completions does the exact same as the code that it is replacing. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * symtab.c (make_file_symbol_completion_list_1): Use add_symtab_completions. --- gdb/symtab.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/gdb/symtab.c b/gdb/symtab.c index b9f4f77..c0fd0fd 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -5391,10 +5391,7 @@ static VEC (char_ptr) * make_file_symbol_completion_list_1 (const char *text, const char *word, const char *srcfile) { - struct symbol *sym; struct symtab *s; - struct block *b; - struct block_iterator iter; /* The symbol we are completing on. Points in same buffer as text. */ const char *sym_text; /* Length of sym_text. */ @@ -5464,18 +5461,9 @@ make_file_symbol_completion_list_1 (const char *text, const char *word, /* Go through this symtab and check the externs and statics for symbols which match. */ - - b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), GLOBAL_BLOCK); - ALL_BLOCK_SYMBOLS (b, iter, sym) - { - COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word); - } - - b = BLOCKVECTOR_BLOCK (SYMTAB_BLOCKVECTOR (s), STATIC_BLOCK); - ALL_BLOCK_SYMBOLS (b, iter, sym) - { - COMPLETION_LIST_ADD_SYMBOL (sym, sym_text, sym_text_len, text, word); - } + add_symtab_completions (SYMTAB_COMPUNIT (s), + sym_text, sym_text_len, + text, word, TYPE_CODE_UNDEF); return (return_val); } -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] symtab.c: Small refactor 2017-02-23 17:24 ` [PATCH v2 1/2] symtab.c: Small refactor Pedro Alves @ 2017-02-23 21:45 ` Yao Qi 2017-02-24 17:45 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Yao Qi @ 2017-02-23 21:45 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 17-02-23 17:24:01, Pedro Alves wrote: > add_symtab_completions does the exact same as the code that it is > replacing. > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * symtab.c (make_file_symbol_completion_list_1): Use > add_symtab_completions. Patch is good to me. -- Yao ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 1/2] symtab.c: Small refactor 2017-02-23 21:45 ` Yao Qi @ 2017-02-24 17:45 ` Pedro Alves 0 siblings, 0 replies; 27+ messages in thread From: Pedro Alves @ 2017-02-24 17:45 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 02/23/2017 09:45 PM, Yao Qi wrote: > On 17-02-23 17:24:01, Pedro Alves wrote: >> add_symtab_completions does the exact same as the code that it is >> replacing. >> >> gdb/ChangeLog: >> yyyy-mm-dd Pedro Alves <palves@redhat.com> >> >> * symtab.c (make_file_symbol_completion_list_1): Use >> add_symtab_completions. > > Patch is good to me. > Thanks, I've pushed this one in. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index 2017-02-23 17:24 ` [PATCH v2 0/2] " Pedro Alves 2017-02-23 17:24 ` [PATCH v2 1/2] symtab.c: Small refactor Pedro Alves @ 2017-02-23 17:24 ` Pedro Alves 2017-02-24 15:34 ` Yao Qi 1 sibling, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-23 17:24 UTC (permalink / raw) To: gdb-patches This patch fixes: -FAIL: gdb.base/completion.exp: tab complete break break.c:ma (timeout) -FAIL: gdb.base/completion.exp: complete break break.c:ma +PASS: gdb.base/completion.exp: tab complete break break.c:ma +PASS: gdb.base/completion.exp: delete breakpoint for tab complete break break.c:ma +PASS: gdb.base/completion.exp: complete break break.c:ma When run with --target_board=dwarf4-gdb-index. The issue here is that make_file_symbol_completion_list_1, used when completing a symbol restricted to a given source file, uses lookup_symtab to look up the symtab with the given name, and search for matching symbols inside. This assumes that there's only one symtab for the given source file. This is an incorrect assumption with (for example) -fdebug-types-section, where we'll have an extra extra symtab containing the types. lookup_symtab finds that symtab, and inside that symtab there are no functions... gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * symtab.c (make_file_symbol_completion_list_1): Iterate over symtabs matching all symtabs with SRCFILE as file name instead of only considering the first hit, with lookup_symtab. --- gdb/symtab.c | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) diff --git a/gdb/symtab.c b/gdb/symtab.c index c0fd0fd..e1a9b8f 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -5391,7 +5391,6 @@ static VEC (char_ptr) * make_file_symbol_completion_list_1 (const char *text, const char *word, const char *srcfile) { - struct symtab *s; /* The symbol we are completing on. Points in same buffer as text. */ const char *sym_text; /* Length of sym_text. */ @@ -5442,28 +5441,15 @@ make_file_symbol_completion_list_1 (const char *text, const char *word, sym_text_len = strlen (sym_text); - /* Find the symtab for SRCFILE (this loads it if it was not yet read - in). */ - s = lookup_symtab (srcfile); - if (s == NULL) + /* Go through symtabs for SRCFILE and check the externs and statics + for symbols which match. */ + iterate_over_symtabs (srcfile, [&] (symtab *s) { - /* Maybe they typed the file with leading directories, while the - symbol tables record only its basename. */ - const char *tail = lbasename (srcfile); - - if (tail > srcfile) - s = lookup_symtab (tail); - } - - /* If we have no symtab for that file, return an empty list. */ - if (s == NULL) - return (return_val); - - /* Go through this symtab and check the externs and statics for - symbols which match. */ - add_symtab_completions (SYMTAB_COMPUNIT (s), - sym_text, sym_text_len, - text, word, TYPE_CODE_UNDEF); + add_symtab_completions (SYMTAB_COMPUNIT (s), + sym_text, sym_text_len, + text, word, TYPE_CODE_UNDEF); + return false; + }); return (return_val); } -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index 2017-02-23 17:24 ` [PATCH v2 2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves @ 2017-02-24 15:34 ` Yao Qi 2017-02-24 17:15 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Yao Qi @ 2017-02-24 15:34 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro Alves <palves@redhat.com> writes: > - /* Find the symtab for SRCFILE (this loads it if it was not yet read > - in). */ > - s = lookup_symtab (srcfile); > - if (s == NULL) > + /* Go through symtabs for SRCFILE and check the externs and statics > + for symbols which match. */ > + iterate_over_symtabs (srcfile, [&] (symtab *s) > { > - /* Maybe they typed the file with leading directories, while the > - symbol tables record only its basename. */ > - const char *tail = lbasename (srcfile); > - > - if (tail > srcfile) > - s = lookup_symtab (tail); > - } > - > - /* If we have no symtab for that file, return an empty list. */ > - if (s == NULL) > - return (return_val); In the original code, lookup_symtab is called twice, and if we inline lookup_symtab here, the change in this patch is more readable. Let us inline lookup_symtab first, the code becomes, s = NULL; iterate_over_symtabs (srcfile, [&] (symtab *symtab) { s = symtab; add_symtab_completions (SYMTAB_COMPUNIT (s), sym_text, sym_text_len, text, word, TYPE_CODE_UNDEF); return true; }); if (s == NULL) { /* Maybe they typed the file with leading directories, while the symbol tables record only its basename. */ const char *tail = lbasename (srcfile); if (tail > srcfile) iterate_over_symtabs (tail, [&] (symtab *symtab) { s = symtab; add_symtab_completions (SYMTAB_COMPUNIT (s), sym_text, sym_text_len, text, word, TYPE_CODE_UNDEF); return true; }); } then, as you described in commit log, we have to iterate all symtabs rather than stop on the first matched symtab. We need to replace "return true;" with "return false;" above. Presumably, this replacement will fix the fails in completion.exp. Then, it turns out that the whole block "if (s == NULL) {...}" is removed by this patch. I'll dig deep to see this block is still needed or not. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index 2017-02-24 15:34 ` Yao Qi @ 2017-02-24 17:15 ` Pedro Alves 0 siblings, 0 replies; 27+ messages in thread From: Pedro Alves @ 2017-02-24 17:15 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches On 02/24/2017 03:34 PM, Yao Qi wrote: > then, as you described in commit log, we have to iterate all symtabs > rather than stop on the first matched symtab. We need to replace > "return true;" with "return false;" above. Presumably, this replacement > will fix the fails in completion.exp. Yeah, that's a good way to put it. > > Then, it turns out that the whole block "if (s == NULL) {...}" is > removed by this patch. I'll dig deep to see this block is still needed > or not. > Here are some tests that fail with current master, due to that second block, but pass with the patch. (I haven't tried to split in two patches.) From 40a60624d4a6efd8560f374fd3e60dde9ac2b610 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Fri, 24 Feb 2017 17:03:01 +0000 Subject: [PATCH] test --- gdb/testsuite/gdb.linespec/base/one/thefile.cc | 5 ++ gdb/testsuite/gdb.linespec/base/two/thefile.cc | 5 ++ gdb/testsuite/gdb.linespec/linespec.exp | 93 +++++++++++++++++++++++++- 3 files changed, 100 insertions(+), 3 deletions(-) diff --git a/gdb/testsuite/gdb.linespec/base/one/thefile.cc b/gdb/testsuite/gdb.linespec/base/one/thefile.cc index 0417b7a..34bc547 100644 --- a/gdb/testsuite/gdb.linespec/base/one/thefile.cc +++ b/gdb/testsuite/gdb.linespec/base/one/thefile.cc @@ -23,3 +23,8 @@ int NameSpace::overload(int x) { return x + 23; } + +int z1 () +{ + return 0; +} diff --git a/gdb/testsuite/gdb.linespec/base/two/thefile.cc b/gdb/testsuite/gdb.linespec/base/two/thefile.cc index 88188a5..264ae97 100644 --- a/gdb/testsuite/gdb.linespec/base/two/thefile.cc +++ b/gdb/testsuite/gdb.linespec/base/two/thefile.cc @@ -24,3 +24,8 @@ int NameSpace::overload(double x) { return (int) x - 23; } + +int z2 () +{ + return 0; +} diff --git a/gdb/testsuite/gdb.linespec/linespec.exp b/gdb/testsuite/gdb.linespec/linespec.exp index ccb73c8..3a7bae5 100644 --- a/gdb/testsuite/gdb.linespec/linespec.exp +++ b/gdb/testsuite/gdb.linespec/linespec.exp @@ -43,6 +43,9 @@ if {$l1 != $l2} { error "somebody incompatibly modified the source files needed by linespec.exp" } +gdb_test_no_output "set breakpoint pending off" \ + "disable pending breakpoints for linespec tests" + # Copying files to a remote host loses the directory prefix during # compilation. if { [is_remote host] } { @@ -55,6 +58,93 @@ if { [is_remote host] } { gdb_test "clear one/thefile.cc:$l1" \ "Deleted breakpoint $decimal *" \ "clear breakpoint using dir/file:line" + + if { [readline_is_used] } { + # There are functions name twodup in both source files. Both + # should be found if we restrict the linespec to the ambiguous + # "thefile.cc" source filename. Check both completion and + # setting the breakpoint. + set tst "complete unique function name in two source files" + send_gdb "break thefile.cc:t\t" + gdb_test_multiple "" $tst { + -re "break thefile.cc:twodup\\(\\) " { + pass $tst + + send_gdb "\n" + gdb_test "" \ + "Breakpoint $decimal at $hex: thefile.cc:twodup\\(\\). \[(\]2 locations\[)\]" \ + "set break at unique function name in two source files" + } + } + + # Check both completing and setting a breakpoint on a linespec + # with a source component, where there's more than one source + # file with the same basename. We should find the functions + # in all matching sources -- one/thefile.cc and + # two/thefile.cc. The "one" file has "z1()", while the "two" + # file has "z2()". + set tst "complete non-unique function name in two source files" + send_gdb "break thefile.cc:z\t" + gdb_test_multiple "" $tst { + -re "break thefile.cc:z\\\x07" { + send_gdb "\t" + gdb_test_multiple "" $tst { + -re "\r\nz1\\(\\)\[ \t\]+z2\\(\\)\[ \t\]+\r\n$gdb_prompt " { + pass $tst + + send_gdb "\n" + gdb_test "" \ + "Function \"z\" not defined in \"thefile.cc\"." \ + "set break at non-unique function name in two source files" + } + } + } + } + + # Now check that disambiguating the source path makes GDB only + # match the symbols in that file. "z" should now have a + # unique completion to "z1()", and setting the breakpoint + # should find only one location. + set tst "complete unique function name in disambiguated source file" + send_gdb "break one/thefile.cc:z\t" + gdb_test_multiple "" $tst { + -re "break one/thefile.cc:z1\\(\\) " { + pass $tst + + send_gdb "\n" + gdb_test "" \ + "Breakpoint $decimal at $hex: file .*thefile.cc, line \[^\r\n\]*" \ + "set break at unique function name in disambiguated source file" + } + } + + # Check that using a non-existing source path does not confuse + # gdb. It should not match any symbol. + set dir_file "one/thefile.cc" + set non_existing "/some/non-existing/absolute/path/prefix/$dir_file" + set non_existing_re [string_to_regexp $non_existing] + + set tst "complete functions in non-existing absolute path" + send_gdb "break $non_existing:\t" + gdb_test_multiple "" $tst { + -re "break $non_existing_re:\\\x07" { + send_gdb "\t\t" + gdb_test_multiple "" $tst { + -re "^\\\x07\\\x07" { + pass $tst + + # There's a function called twodup in each of + # the thefile.cc files. Make sure none is + # picked. + send_gdb "twodup\n" + gdb_test "" \ + "No source file named $non_existing_re." \ + "set break in function in non-existing absolute path" + } + } + } + } + } } gdb_test "break thefile.cc:$l1" \ @@ -73,9 +163,6 @@ gdb_test "break dupname:label" \ # not the locations. gdb_test "complete condition " "condition $decimal\r\ncondition $decimal\r\ncondition $decimal" -gdb_test_no_output "set breakpoint pending off" \ - "disable pending breakpoints for linespec tests" - # This is PR breakpoints/12856. gdb_test "break lspec.cc:nosuchfunction" \ "Function \"nosuchfunction\" not defined in \"lspec.cc\"." \ -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/3] Introduce gdb::function_view 2017-02-22 14:50 [PATCH 0/3] Introduce gdb::function_view & fix completion bug Pedro Alves 2017-02-22 14:50 ` [PATCH 2/3] Use gdb::function_view in iterate_over_symtabs & co Pedro Alves 2017-02-22 14:50 ` [PATCH 3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves @ 2017-02-22 14:50 ` Pedro Alves 2017-02-22 15:15 ` Simon Marchi 2 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-22 14:50 UTC (permalink / raw) To: gdb-patches This commit adds new function_view type. This type holds a a non-owning reference to a callable. It is meant to be used as callback type of functions, instead of using C-style pair of function pointer and 'void *data' arguments. function_view allows passing references to stateful function objects / lambdas w/ captures as callbacks efficiently, while function pointer + 'void *' does not. See the intro in the new function-view.h header for more. Unit tests included. I added a new gdb/unittests/ subdir this time, instead of putting the tests under gdb/. If this is agreed to be a good idea, some of the current selftests that exercise gdb/common/ things but live in gdb/ could move here (e.g., gdb/utils-selftests.c). gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS): New. (SFILES): Add $(SUBDIR_UNITTEST_SRCS). (COMMON_OBS): Add $(SUBDIR_UNITTEST_OBS). (%.o) <unittests/%.c>: New pattern. (INIT_FILES): Add $(SUBDIR_UNITTESTS_SRCS). * common/function-view.h: New file. * unittests/function-view-selftests.c: New file. --- gdb/Makefile.in | 24 ++- gdb/common/function-view.h | 320 ++++++++++++++++++++++++++++++++ gdb/unittests/function-view-selftests.c | 183 ++++++++++++++++++ 3 files changed, 524 insertions(+), 3 deletions(-) create mode 100644 gdb/common/function-view.h create mode 100644 gdb/unittests/function-view-selftests.c diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 43253d3..a4cac36 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -523,6 +523,12 @@ SUBDIR_PYTHON_DEPS = SUBDIR_PYTHON_LDFLAGS = SUBDIR_PYTHON_CFLAGS = +SUBDIR_UNITTESTS_SRCS = \ + unittests/function-view-selftests.c + +SUBDIR_UNITTESTS_OBS = \ + function-view-selftests.o + # Opcodes currently live in one of two places. Either they are in the # opcode library, typically ../opcodes, or they are in a header file # in INCLUDE_DIR. @@ -1216,7 +1222,8 @@ SFILES = \ common/xml-utils.c \ mi/mi-common.c \ target/waitstatus.c \ - $(SUBDIR_GCC_COMPILE_SRCS) + $(SUBDIR_GCC_COMPILE_SRCS) \ + $(SUBDIR_UNITTEST_SRCS) LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c @@ -1800,7 +1807,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ xml-syscall.o \ xml-tdesc.o \ xml-utils.o \ - $(SUBDIR_GCC_COMPILE_OBS) + $(SUBDIR_GCC_COMPILE_OBS) \ + $(SUBDIR_UNITTESTS_OBS) TSOBS = inflow.o @@ -1909,6 +1917,10 @@ all: gdb$(EXEEXT) $(CONFIG_ALL) $(COMPILE) $< $(POSTCOMPILE) +%.o: ${srcdir}/unittests/%.c + $(COMPILE) $< + $(POSTCOMPILE) + # Specify an explicit rule for gdb/common/agent.c, to avoid a clash with the # object file generate by gdb/agent.c. common-agent.o: $(srcdir)/common/agent.c @@ -2124,7 +2136,13 @@ test-cp-name-parser$(EXEEXT): test-cp-name-parser.o $(LIBIBERTY) # duplicates. Files in the gdb/ directory can end up appearing in # COMMON_OBS (as a .o file) and CONFIG_SRCS (as a .c file). -INIT_FILES = $(COMMON_OBS) $(TSOBS) $(CONFIG_SRCS) $(SUBDIR_GCC_COMPILE_SRCS) +INIT_FILES = \ + $(COMMON_OBS) \ + $(TSOBS) \ + $(CONFIG_SRCS) \ + $(SUBDIR_GCC_COMPILE_SRCS) \ + $(SUBDIR_UNITTESTS_SRCS) + init.c: $(INIT_FILES) @echo Making init.c @rm -f init.c-tmp init.l-tmp diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h new file mode 100644 index 0000000..cd455f8 --- /dev/null +++ b/gdb/common/function-view.h @@ -0,0 +1,320 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifndef COMMON_FUNCTION_VIEW_H +#define COMMON_FUNCTION_VIEW_H + +/* function_view is a polymorphic type-erasing wrapper class that + encapsulates a non-owning reference to arbitrary callable objects. + + A way to put it is that function_view is to std::function like + std::string_view is to std::string. While std::function stores a + type-erased callable object internally, function_view holds a + type-erased reference to an external callable object. + + This is meant to be used as callback type of a function that: + + #1 - Takes a callback as parameter. + + #2 - Does not store the callback anywhere; instead if just calls + it or forwards it to some other function that calls it. + + #3 - When we don't want, or can't make said function be a + template function with the callable type as template + parameter. For example, when the callback is a parameter of + a virtual member function, or when putting the function + template in a header would expose too much implementation + detail. + + For this use case, which is quite pervasive, a function_view is a + better choice for callback type than std::function. It is a better + choice because std::function is a heavy-weight object with value + semantics that generally requires a heap allocation on + construction/assignment of the target callable, while function_view + is light and does not require any heap allocation. It _is_ + possible to use std::function in such a way that avoids most of the + overhead by making sure to only construct it with callables of + types that fit std::function's small object optimization, such as + function pointers and std::reference_wrapper callables, however, + that is quite inconvenient in practice, because restricting to + free-function callables would imply no state/capture (which we need + in most cases), and std::reference_wrapper implies remembering to + use std::ref/std::cref where the callable is constructed, with the + added inconvenience that those function have deleted rvalue-ref + overloads, meaning you can't use unnamed/temporary lambdas with + them. + + Note that because function_view is a non-owning view of a callable, + care must be taken to ensure that the callable outlives the + function_view that calls it. This is not really a problem for the + use case function_view is intended for, such as passing a temporary + function object / lambda to a function that accepts a callback, + because in those cases, the temporary is guaranteed to be live + until the called function returns. + + Calling a function_view with no associated target is undefined, + unlike with std::function, which throws bad_function_call. This is + by design, to avoid the otherwise necessary NULL check in + function_view::operator(). + + Since function_view objects are small (a pair of pointers), they + should generally be passed around by value. + + Usage: + + Given this function that accepts a callback: + + void + iterate_over_foos (gdb::function_view<void (foo *)> callback) + { + for (auto & : foos) + callback (&foo); + } + + you can call it like this, passing a lambda as callback: + + iterate_over_foos ([&] (foo *f) { + process_one_foo (f); + }); + + or like this, passing a function object as callback: + + struct function_object + { + void operator() (foo *f) + { + if (s->check ()) + process_one_foo (f); + } + + // some state + state *s; + }; + + function_object matcher (mystate); + iterate_over_foos (matcher); + + or like this, passing a function pointer as callback: + + iterate_over_foos (process_one_foo); + + You can find unit tests covering the whole API in + unittests/function-view-selftests.c. */ + +namespace gdb { + +namespace traits +{ + /* A few trait helpers. */ + template<typename Predicate> + struct Not : public std::integral_constant<bool, !Predicate::value> + {}; + + template<typename...> + struct Or; + + template<> + struct Or<> : public std::false_type + {}; + + template<typename B1> + struct Or<B1> : public B1 + {}; + + template<typename B1, typename B2> + struct Or<B1, B2> + : public std::conditional<B1::value, B1, B2>::type + {}; + + template<typename B1,typename B2,typename B3, typename... Bn> + struct Or<B1, B2, B3, Bn...> + : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type + {}; +} + +namespace fv_detail { +/* Bits shared by all function_view instantiations that do not depend + on the template parameters. */ + +/* Storage for the erased callable. This is a union in order to be + able to save both a function object (data) pointer or a function + pointer without triggering undefined behavior. */ +union erased_callable +{ + /* For function objects. */ + void *data; + + /* For function pointers. */ + void (*fn) (); +}; + +} /* namespace fv_detail */ + +/* Use partial specialization to get access to the callable's + signature. */ +template<class Signature> +struct function_view; + +template<typename Res, typename... Args> +class function_view<Res (Args...)> +{ + template<typename From, typename To> + using CompatibleReturnType + = traits::Or<std::is_void<To>, + std::is_same<From, To>, + std::is_convertible<From, To>>; + + /* True if Func can be called with Args, and the result, and the + result is convertible to Res, unless Res is void. */ + template<typename Callable, + typename Res2 = typename std::result_of<Callable &(Args...)>::type> + struct IsCompatibleCallable : CompatibleReturnType<Res2, Res> + {}; + + /* True if Callable is a function_view. Used to avoid hijacking the + copy ctor. */ + template <typename Callable> + struct IsFunctionView + : std::is_same<function_view, typename std::decay<Callable>::type> + {}; + + /* Helper to make SFINAE logic easier to read. */ + template<typename Condition> + using Requires = typename std::enable_if<Condition::value, void>::type; + + public: + + /* NULL by default. */ + constexpr function_view () noexcept + : m_erased_callable {}, + m_invoker {} + {} + + /* Default copy/assignment is fine. */ + function_view (const function_view &) = default; + function_view &operator= (const function_view &) = default; + + /* This is the main entry point. Use SFINAE to avoid hijacking the + copy constructor and to ensure that the target type is + compatible. */ + template + <typename Callable, + typename = Requires<traits::Not<IsFunctionView<Callable>>>, + typename = Requires<IsCompatibleCallable<Callable>>> + function_view (Callable &&callable) noexcept + { + bind (callable); + } + + /* Construct a NULL function_view. */ + constexpr function_view (std::nullptr_t) noexcept + : m_erased_callable {}, + m_invoker {} + {} + + /* Clear a function_view. */ + function_view &operator= (std::nullptr_t) noexcept + { + m_invoker = nullptr; + return *this; + } + + /* Return true if the wrapper has a target, false otherwise. Note + we check M_INVOKER instead of M_ERASED_CALLABLE because we don't + know which member of the union is active right now. */ + constexpr explicit operator bool () const noexcept + { return m_invoker != nullptr; } + + /* Call the callable. */ + Res operator () (Args... args) const + { return m_invoker (m_erased_callable, std::forward<Args> (args)...); } + + private: + + /* Bind this function_view to a compatible function object + reference. */ + template <typename Callable> + void bind (Callable &callable) noexcept + { + m_erased_callable.data = (void *) std::addressof (callable); + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) + noexcept (noexcept (callable (std::forward<Args> (args)...))) -> Res + { + auto &restored_callable = *static_cast<Callable *> (ecall.data); + /* The explicit cast to Res avoids a compile error when Res is + void and the callable returns non-void. */ + return (Res) restored_callable (std::forward<Args> (args)...); + }; + } + + /* Bind this function_view to a compatible function pointer. + + Making this a separate function allows avoiding one indirection, + by storing the function pointer directly in the storage, instead + of a pointer to pointer. erased_callable is then a union in + order to avoid storing a function pointer as a data pointer here, + which would be undefined. */ + template<class Res2, typename... Args2> + void bind (Res2 (*fn) (Args2...)) noexcept + { + m_erased_callable.fn = reinterpret_cast<void (*) ()> (fn); + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) + noexcept (noexcept (fn (std::forward<Args> (args)...))) -> Res + { + auto restored_fn = reinterpret_cast<Res2 (*) (Args2...)> (ecall.fn); + /* The explicit cast to Res avoids a compile error when Res is + void and the callable returns non-void. */ + return (Res) restored_fn (std::forward<Args> (args)...); + }; + } + + /* Storage for the erased callable. */ + fv_detail::erased_callable m_erased_callable; + + /* The invoker. This is set to a capture-less lambda by one of the + 'bind' overloads. The lambda restores the right type of the + callable (which is passed as first argument), and forwards the + args. */ + Res (*m_invoker) (fv_detail::erased_callable, Args...); +}; + +/* Allow comparison with NULL. Defer the work to the in-class + operator bool implementation. */ + +template<typename Res, typename... Args> +constexpr inline bool +operator== (const function_view<Res (Args...)> &f, std::nullptr_t) noexcept +{ return !static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator== (std::nullptr_t, const function_view<Res (Args...)> &f) noexcept +{ return !static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator!= (const function_view<Res (Args...)> &f, std::nullptr_t) noexcept +{ return static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator!= (std::nullptr_t, const function_view<Res (Args...)> &f) noexcept +{ return static_cast<bool> (f); } + +} /* namespace gdb */ + +#endif diff --git a/gdb/unittests/function-view-selftests.c b/gdb/unittests/function-view-selftests.c new file mode 100644 index 0000000..8f73bc4 --- /dev/null +++ b/gdb/unittests/function-view-selftests.c @@ -0,0 +1,183 @@ +/* Self tests for function_view for GDB, the GNU debugger. + + Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include "defs.h" +#include "selftest.h" +#include "common/function-view.h" + +#if GDB_SELF_TEST + +namespace selftests { +namespace function_view { + +static int +add_one (int count) +{ + return ++count; +} + +static short +add_one_short (short count) +{ + return ++count; +} + +static int +call_callback (int count, gdb::function_view <int(int)> callback) +{ + return callback (count); +} + +static void +call_callback_void (int count, gdb::function_view <void(int)> callback) +{ + callback (count); +} + +struct plus_one_function_object +{ + int operator () (int count) + { + ++calls; + return ++count; + } + + int calls = 0; +}; + +static void +run_tests () +{ + /* A simple lambda. */ + auto plus_one = [] (int count) { return ++count; }; + + /* A function_view that references the lambda. */ + gdb::function_view<int (int)> plus_one_view (plus_one); + + /* Check calling the lambda directly. */ + SELF_CHECK (plus_one (0) == 1); + SELF_CHECK (plus_one (1) == 2); + + /* Check calling lambda via the view. */ + SELF_CHECK (plus_one_view (2) == 3); + SELF_CHECK (plus_one_view (3) == 4); + + /* Check calling a function that takes a function_view as argument, + by value. Pass a lambda, making sure a function_view is properly + constructed implicitly. */ + SELF_CHECK (call_callback (1, [] (int count) + { + return count + 2; + }) == 3); + + /* Same, passing a named/lvalue lambda. */ + SELF_CHECK (call_callback (1, plus_one) == 2); + /* Same, passing named/lvalue function_view (should copy). */ + SELF_CHECK (call_callback (1, plus_one_view) == 2); + + /* Check constructing a function view over a function-object + callable, and calling it. */ + plus_one_function_object func_obj; + SELF_CHECK (func_obj (0) == 1); + SELF_CHECK (call_callback (1, func_obj) == 2); + /* Check that the callable was referenced, not copied. */ + SELF_CHECK (func_obj.calls == 2); + + /* Check constructing a function_view over free-function callable, + and calling it. */ + SELF_CHECK (call_callback (1, add_one) == 2); + + /* Check calling a function with a + compatible-but-not-exactly-the-same prototype. */ + SELF_CHECK (call_callback (1, [] (short count) -> short + { + return count + 2; + }) == 3); + /* Same, but passing a function pointer. */ + SELF_CHECK (call_callback (1, add_one_short) == 2); + + /* Like std::function, a function_view that expects a void return + can reference callables with non-void return type. The result is + simply discarded. Check a lambda, function object and a function + pointer. */ + call_callback_void (1, [] (int count) -> int + { + return count + 2; + }); + call_callback_void (1, func_obj); + call_callback_void (1, add_one); + + /* Check that the main ctor doesn't hijack the copy ctor. */ + auto plus_one_view2 (plus_one_view); + auto plus_one_view3 (plus_one_view2); + static_assert (std::is_same<decltype (plus_one_view), + decltype (plus_one_view2)>::value, ""); + static_assert (std::is_same<decltype (plus_one_view), + decltype (plus_one_view3)>::value, ""); + + SELF_CHECK (plus_one_view3 (1) == 2); + + /* Likewise, but propagate a NULL callable. If this calls the main + function_view ctor instead of the copy ctor by mistake, then + null_func_2 ends up non-NULL (because it'd instead reference + null_func_1 as just another callable). */ + constexpr gdb::function_view<int(int)> null_func_1 = nullptr; + constexpr auto null_func_2 (null_func_1); + + /* While at it, check whether the function_view is bound using + various forms, op==, op!= and op bool. */ + + /* op== */ + static_assert (null_func_2 == nullptr, ""); + static_assert (nullptr == null_func_2, ""); + static_assert (null_func_2 == NULL, ""); + static_assert (NULL == null_func_2, ""); + + /* op!= */ + static_assert (!(null_func_2 != nullptr), ""); + static_assert (!(nullptr != null_func_2), ""); + static_assert (!(null_func_2 != NULL), ""); + static_assert (!(NULL != null_func_2), ""); + + /* op bool */ + static_assert (!null_func_2, ""); + + /* Check the nullptr_t ctor. */ + constexpr gdb::function_view<int(int)> check_ctor_nullptr (nullptr); + static_assert (!check_ctor_nullptr, ""); + + /* Check the nullptr_t op= */ + gdb::function_view<int(int)> check_op_eq_null (add_one); + SELF_CHECK (check_op_eq_null); + check_op_eq_null = nullptr; + SELF_CHECK (!check_op_eq_null); +} + +} /* namespace function_view */ +} /* namespace selftests */ + +#endif + +void +_initialize_function_view_selftests () +{ +#if GDB_SELF_TEST + register_self_test (selftests::function_view::run_tests); +#endif +} -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] Introduce gdb::function_view 2017-02-22 14:50 ` [PATCH 1/3] Introduce gdb::function_view Pedro Alves @ 2017-02-22 15:15 ` Simon Marchi 2017-02-22 17:40 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Simon Marchi @ 2017-02-22 15:15 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 2017-02-22 09:50, Pedro Alves wrote: > This commit adds new function_view type. This type holds a a > non-owning reference to a callable. It is meant to be used as > callback type of functions, instead of using C-style pair of function > pointer and 'void *data' arguments. function_view allows passing > references to stateful function objects / lambdas w/ captures as > callbacks efficiently, while function pointer + 'void *' does not. > > See the intro in the new function-view.h header for more. > > Unit tests included. I added a new gdb/unittests/ subdir this time, > instead of putting the tests under gdb/. If this is agreed to be a > good idea, some of the current selftests that exercise gdb/common/ > things but live in gdb/ could move here (e.g., gdb/utils-selftests.c). > > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS): New. > (SFILES): Add $(SUBDIR_UNITTEST_SRCS). > (COMMON_OBS): Add $(SUBDIR_UNITTEST_OBS). > (%.o) <unittests/%.c>: New pattern. > (INIT_FILES): Add $(SUBDIR_UNITTESTS_SRCS). > * common/function-view.h: New file. > * unittests/function-view-selftests.c: New file. > --- > gdb/Makefile.in | 24 ++- > gdb/common/function-view.h | 320 > ++++++++++++++++++++++++++++++++ > gdb/unittests/function-view-selftests.c | 183 ++++++++++++++++++ > 3 files changed, 524 insertions(+), 3 deletions(-) > create mode 100644 gdb/common/function-view.h > create mode 100644 gdb/unittests/function-view-selftests.c > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 43253d3..a4cac36 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -523,6 +523,12 @@ SUBDIR_PYTHON_DEPS = > SUBDIR_PYTHON_LDFLAGS = > SUBDIR_PYTHON_CFLAGS = > > +SUBDIR_UNITTESTS_SRCS = \ > + unittests/function-view-selftests.c > + > +SUBDIR_UNITTESTS_OBS = \ > + function-view-selftests.o > + > # Opcodes currently live in one of two places. Either they are in the > # opcode library, typically ../opcodes, or they are in a header file > # in INCLUDE_DIR. > @@ -1216,7 +1222,8 @@ SFILES = \ > common/xml-utils.c \ > mi/mi-common.c \ > target/waitstatus.c \ > - $(SUBDIR_GCC_COMPILE_SRCS) > + $(SUBDIR_GCC_COMPILE_SRCS) \ > + $(SUBDIR_UNITTEST_SRCS) > > LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c > > @@ -1800,7 +1807,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ > xml-syscall.o \ > xml-tdesc.o \ > xml-utils.o \ > - $(SUBDIR_GCC_COMPILE_OBS) > + $(SUBDIR_GCC_COMPILE_OBS) \ > + $(SUBDIR_UNITTESTS_OBS) > > TSOBS = inflow.o > > @@ -1909,6 +1917,10 @@ all: gdb$(EXEEXT) $(CONFIG_ALL) > $(COMPILE) $< > $(POSTCOMPILE) > > +%.o: ${srcdir}/unittests/%.c > + $(COMPILE) $< > + $(POSTCOMPILE) > + > # Specify an explicit rule for gdb/common/agent.c, to avoid a clash > with the > # object file generate by gdb/agent.c. > common-agent.o: $(srcdir)/common/agent.c > @@ -2124,7 +2136,13 @@ test-cp-name-parser$(EXEEXT): > test-cp-name-parser.o $(LIBIBERTY) > # duplicates. Files in the gdb/ directory can end up appearing in > # COMMON_OBS (as a .o file) and CONFIG_SRCS (as a .c file). > > -INIT_FILES = $(COMMON_OBS) $(TSOBS) $(CONFIG_SRCS) > $(SUBDIR_GCC_COMPILE_SRCS) > +INIT_FILES = \ > + $(COMMON_OBS) \ > + $(TSOBS) \ > + $(CONFIG_SRCS) \ > + $(SUBDIR_GCC_COMPILE_SRCS) \ > + $(SUBDIR_UNITTESTS_SRCS) > + > init.c: $(INIT_FILES) > @echo Making init.c > @rm -f init.c-tmp init.l-tmp > diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h > new file mode 100644 > index 0000000..cd455f8 > --- /dev/null > +++ b/gdb/common/function-view.h > @@ -0,0 +1,320 @@ > +/* Copyright (C) 2017 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or > modify > + it under the terms of the GNU General Public License as published > by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > <http://www.gnu.org/licenses/>. */ > + > +#ifndef COMMON_FUNCTION_VIEW_H > +#define COMMON_FUNCTION_VIEW_H > + > +/* function_view is a polymorphic type-erasing wrapper class that > + encapsulates a non-owning reference to arbitrary callable objects. > + > + A way to put it is that function_view is to std::function like > + std::string_view is to std::string. While std::function stores a > + type-erased callable object internally, function_view holds a > + type-erased reference to an external callable object. > + > + This is meant to be used as callback type of a function that: > + > + #1 - Takes a callback as parameter. > + > + #2 - Does not store the callback anywhere; instead if just calls if -> it > + it or forwards it to some other function that calls it. > + > + #3 - When we don't want, or can't make said function be a > + template function with the callable type as template > + parameter. For example, when the callback is a parameter of > + a virtual member function, or when putting the function > + template in a header would expose too much implementation > + detail. > + > + For this use case, which is quite pervasive, a function_view is a > + better choice for callback type than std::function. It is a better > + choice because std::function is a heavy-weight object with value > + semantics that generally requires a heap allocation on > + construction/assignment of the target callable, while function_view > + is light and does not require any heap allocation. It _is_ > + possible to use std::function in such a way that avoids most of the > + overhead by making sure to only construct it with callables of > + types that fit std::function's small object optimization, such as > + function pointers and std::reference_wrapper callables, however, > + that is quite inconvenient in practice, because restricting to > + free-function callables would imply no state/capture (which we need > + in most cases), and std::reference_wrapper implies remembering to > + use std::ref/std::cref where the callable is constructed, with the > + added inconvenience that those function have deleted rvalue-ref > + overloads, meaning you can't use unnamed/temporary lambdas with > + them. > + > + Note that because function_view is a non-owning view of a callable, > + care must be taken to ensure that the callable outlives the > + function_view that calls it. This is not really a problem for the > + use case function_view is intended for, such as passing a temporary > + function object / lambda to a function that accepts a callback, > + because in those cases, the temporary is guaranteed to be live > + until the called function returns. > + > + Calling a function_view with no associated target is undefined, > + unlike with std::function, which throws bad_function_call. This is > + by design, to avoid the otherwise necessary NULL check in > + function_view::operator(). > + > + Since function_view objects are small (a pair of pointers), they > + should generally be passed around by value. > + > + Usage: > + > + Given this function that accepts a callback: It's not necessary, but it would be nice to have the equivalent example of how it would've been done before (with a function pointer) so that people can relate the following example to something they already know. > + void > + iterate_over_foos (gdb::function_view<void (foo *)> callback) > + { > + for (auto & : foos) I think you're missing a "foo" here. > + callback (&foo); > + } > + > + you can call it like this, passing a lambda as callback: > + > + iterate_over_foos ([&] (foo *f) { > + process_one_foo (f); > + }); > + > + or like this, passing a function object as callback: > + > + struct function_object > + { > + void operator() (foo *f) > + { > + if (s->check ()) > + process_one_foo (f); > + } > + > + // some state > + state *s; > + }; > + > + function_object matcher (mystate); > + iterate_over_foos (matcher); > + > + or like this, passing a function pointer as callback: > + > + iterate_over_foos (process_one_foo); > + > + You can find unit tests covering the whole API in > + unittests/function-view-selftests.c. */ > + > +namespace gdb { > + > +namespace traits > +{ Is it intended to have this { on a separate line, unlike the other namespace declarations? > + /* A few trait helpers. */ > + template<typename Predicate> > + struct Not : public std::integral_constant<bool, !Predicate::value> > + {}; > + > + template<typename...> > + struct Or; > + > + template<> > + struct Or<> : public std::false_type > + {}; > + > + template<typename B1> > + struct Or<B1> : public B1 > + {}; > + > + template<typename B1, typename B2> > + struct Or<B1, B2> > + : public std::conditional<B1::value, B1, B2>::type > + {}; > + > + template<typename B1,typename B2,typename B3, typename... Bn> > + struct Or<B1, B2, B3, Bn...> > + : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type > + {}; > +} > + > +namespace fv_detail { > +/* Bits shared by all function_view instantiations that do not depend > + on the template parameters. */ > + > +/* Storage for the erased callable. This is a union in order to be > + able to save both a function object (data) pointer or a function > + pointer without triggering undefined behavior. */ > +union erased_callable > +{ > + /* For function objects. */ > + void *data; > + > + /* For function pointers. */ > + void (*fn) (); > +}; > + > +} /* namespace fv_detail */ > + > +/* Use partial specialization to get access to the callable's > + signature. */ > +template<class Signature> > +struct function_view; > + > +template<typename Res, typename... Args> > +class function_view<Res (Args...)> > +{ > + template<typename From, typename To> > + using CompatibleReturnType > + = traits::Or<std::is_void<To>, > + std::is_same<From, To>, > + std::is_convertible<From, To>>; > + > + /* True if Func can be called with Args, and the result, and the > + result is convertible to Res, unless Res is void. */ > + template<typename Callable, > + typename Res2 = typename std::result_of<Callable > &(Args...)>::type> > + struct IsCompatibleCallable : CompatibleReturnType<Res2, Res> > + {}; > + > + /* True if Callable is a function_view. Used to avoid hijacking the > + copy ctor. */ > + template <typename Callable> > + struct IsFunctionView > + : std::is_same<function_view, typename std::decay<Callable>::type> > + {}; > + > + /* Helper to make SFINAE logic easier to read. */ > + template<typename Condition> > + using Requires = typename std::enable_if<Condition::value, > void>::type; > + > + public: > + > + /* NULL by default. */ > + constexpr function_view () noexcept > + : m_erased_callable {}, > + m_invoker {} > + {} > + > + /* Default copy/assignment is fine. */ > + function_view (const function_view &) = default; > + function_view &operator= (const function_view &) = default; > + > + /* This is the main entry point. Use SFINAE to avoid hijacking the > + copy constructor and to ensure that the target type is > + compatible. */ > + template > + <typename Callable, > + typename = Requires<traits::Not<IsFunctionView<Callable>>>, > + typename = Requires<IsCompatibleCallable<Callable>>> > + function_view (Callable &&callable) noexcept > + { > + bind (callable); > + } > + > + /* Construct a NULL function_view. */ > + constexpr function_view (std::nullptr_t) noexcept > + : m_erased_callable {}, > + m_invoker {} > + {} > + > + /* Clear a function_view. */ > + function_view &operator= (std::nullptr_t) noexcept > + { > + m_invoker = nullptr; > + return *this; > + } > + > + /* Return true if the wrapper has a target, false otherwise. Note > + we check M_INVOKER instead of M_ERASED_CALLABLE because we don't > + know which member of the union is active right now. */ > + constexpr explicit operator bool () const noexcept > + { return m_invoker != nullptr; } > + > + /* Call the callable. */ > + Res operator () (Args... args) const > + { return m_invoker (m_erased_callable, std::forward<Args> > (args)...); } > + > + private: > + > + /* Bind this function_view to a compatible function object > + reference. */ > + template <typename Callable> > + void bind (Callable &callable) noexcept > + { > + m_erased_callable.data = (void *) std::addressof (callable); > + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) > + noexcept (noexcept (callable (std::forward<Args> (args)...))) -> > Res > + { > + auto &restored_callable = *static_cast<Callable *> (ecall.data); > + /* The explicit cast to Res avoids a compile error when Res is > + void and the callable returns non-void. */ > + return (Res) restored_callable (std::forward<Args> (args)...); > + }; > + } > + > + /* Bind this function_view to a compatible function pointer. > + > + Making this a separate function allows avoiding one indirection, > + by storing the function pointer directly in the storage, instead > + of a pointer to pointer. erased_callable is then a union in > + order to avoid storing a function pointer as a data pointer here, > + which would be undefined. */ > + template<class Res2, typename... Args2> > + void bind (Res2 (*fn) (Args2...)) noexcept > + { > + m_erased_callable.fn = reinterpret_cast<void (*) ()> (fn); > + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) > + noexcept (noexcept (fn (std::forward<Args> (args)...))) -> Res > + { > + auto restored_fn = reinterpret_cast<Res2 (*) (Args2...)> (ecall.fn); > + /* The explicit cast to Res avoids a compile error when Res is > + void and the callable returns non-void. */ > + return (Res) restored_fn (std::forward<Args> (args)...); > + }; > + } > + > + /* Storage for the erased callable. */ > + fv_detail::erased_callable m_erased_callable; > + > + /* The invoker. This is set to a capture-less lambda by one of the > + 'bind' overloads. The lambda restores the right type of the > + callable (which is passed as first argument), and forwards the > + args. */ > + Res (*m_invoker) (fv_detail::erased_callable, Args...); > +}; > + > +/* Allow comparison with NULL. Defer the work to the in-class > + operator bool implementation. */ > + > +template<typename Res, typename... Args> > +constexpr inline bool > +operator== (const function_view<Res (Args...)> &f, std::nullptr_t) > noexcept > +{ return !static_cast<bool> (f); } > + > +template<typename Res, typename... Args> > +constexpr inline bool > +operator== (std::nullptr_t, const function_view<Res (Args...)> &f) > noexcept > +{ return !static_cast<bool> (f); } > + > +template<typename Res, typename... Args> > +constexpr inline bool > +operator!= (const function_view<Res (Args...)> &f, std::nullptr_t) > noexcept > +{ return static_cast<bool> (f); } > + > +template<typename Res, typename... Args> > +constexpr inline bool > +operator!= (std::nullptr_t, const function_view<Res (Args...)> &f) > noexcept > +{ return static_cast<bool> (f); } > + > +} /* namespace gdb */ > + > +#endif I am not going to try to understand any of this... but as long as it works I'm happy. Simon ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] Introduce gdb::function_view 2017-02-22 15:15 ` Simon Marchi @ 2017-02-22 17:40 ` Pedro Alves 2017-02-22 17:49 ` [PATCH v1.1 " Pedro Alves 2017-02-22 18:02 ` [PATCH " Simon Marchi 0 siblings, 2 replies; 27+ messages in thread From: Pedro Alves @ 2017-02-22 17:40 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On 02/22/2017 03:15 PM, Simon Marchi wrote: >> + >> + This is meant to be used as callback type of a function that: >> + >> + #1 - Takes a callback as parameter. >> + >> + #2 - Does not store the callback anywhere; instead if just calls > > if -> it > >> + it or forwards it to some other function that calls it. Thanks. Maybe that's too many confusing "it"s. I've rewritten it to #2 - Does not store the callback anywhere; instead the function just calls the callback directly or forwards it to some other function that calls it. >> + >> + Usage: >> + >> + Given this function that accepts a callback: > > It's not necessary, but it would be nice to have the equivalent example > of how it would've been done before (with a function pointer) so that > people can relate the following example to something they already know. I wasn't sure whether that should be here, thinking it might be more appropriate for the internals manual? I would paste an example using a callable as template parameter there too, for example. But I guess writing it too can't hurt, since that's likely where people will look first when they want to know what are those "function_view"s in the source. I've extended this section in that direction, and tried to clarify other bits along with it. See below. > >> + void >> + iterate_over_foos (gdb::function_view<void (foo *)> callback) >> + { >> + for (auto & : foos) > > I think you're missing a "foo" here. Indeed. Found another buglet in the examples that I fixed too. >> +namespace traits >> +{ > > Is it intended to have this { on a separate line, unlike the other > namespace declarations? Nope, somehow missed it. > I am not going to try to understand any of this... but as long as it > works I'm happy. Yeah, these library facilities tend to require use of C++ that is not normally used by "regular" code. In isolation, none of the features used is really complicated, but when they're all combined, it gives the impression otherwise. The advantage is that it ends up all contained in a single place, while users of the library end up being simple and efficient. Let me know if I can explain things better. Here's the tweak I mean to squash down addressing your review. I also realized that the unit test had some formatting issues, and the symbol names uses there could be renamed to make things a bit clearer. I'll send the updated (squashed) patch as a reply. From 79ce1e9b5fc1e69747d033fc384c7f50c61be1ba Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 22 Feb 2017 17:30:00 +0000 Subject: [PATCH] review --- gdb/common/function-view.h | 104 ++++++++++++++++++++---------- gdb/unittests/function-view-selftests.c | 109 ++++++++++++++++---------------- 2 files changed, 125 insertions(+), 88 deletions(-) diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h index 3303783..af2593f 100644 --- a/gdb/common/function-view.h +++ b/gdb/common/function-view.h @@ -30,33 +30,67 @@ #1 - Takes a callback as parameter. - #2 - Does not store the callback anywhere; instead if just calls - it or forwards it to some other function that calls it. - - #3 - When we don't want, or can't make said function be a - template function with the callable type as template - parameter. For example, when the callback is a parameter of - a virtual member function, or when putting the function - template in a header would expose too much implementation - detail. - - For this use case, which is quite pervasive, a function_view is a - better choice for callback type than std::function. It is a better - choice because std::function is a heavy-weight object with value + #2 - Wants to support arbitrary callable objects as callback type + (e.g., stateful function objects, lambda closures, free + functions). + + #3 - Does not store the callback anywhere; instead the function + just calls the callback directly or forwards it to some + other function that calls it. + + #4 - Can't be, or we don't want it to be, a template function + with the callable type as template parameter. For example, + when the callback is a parameter of a virtual member + function, or when putting the function template in a header + would expose too much implementation detail. + + Note that the C-style "function pointer" + "void *data" callback + parameter idiom fails requirement #2 above. Please don't add new + uses of that idiom. I.e., something like this wouldn't work; + + typedef bool (iterate_over_foos_cb) (foo *f, void *user_data), + void iterate_over_foos (iterate_over_foos_cb *callback, void *user_data); + + foo *find_foo_by_type (int type) + { + foo *found = nullptr; + + iterate_over_foos ([&] (foo *f, void *data) + { + if (foo->type == type) + { + found = foo; + return true; // stop iterating + } + return false; // continue iterating + }, NULL); + + return found; + } + + The above wouldn't compile, because lambdas with captures can't be + implicitly converted to a function pointer (because a capture means + some context data must be passed to the lambda somehow). + + C++11 gave us std::function as type-erased wrapper around arbitrary + callables, however, std::function is not an ideal fit for transient + callbacks such as the use case above. For this use case, which is + quite pervasive, a function_view is a better choice, because while + while function_view is light and does not require any heap + allocation, std::function is a heavy-weight object with value semantics that generally requires a heap allocation on - construction/assignment of the target callable, while function_view - is light and does not require any heap allocation. It _is_ - possible to use std::function in such a way that avoids most of the - overhead by making sure to only construct it with callables of - types that fit std::function's small object optimization, such as - function pointers and std::reference_wrapper callables, however, - that is quite inconvenient in practice, because restricting to - free-function callables would imply no state/capture (which we need - in most cases), and std::reference_wrapper implies remembering to - use std::ref/std::cref where the callable is constructed, with the - added inconvenience that those function have deleted rvalue-ref - overloads, meaning you can't use unnamed/temporary lambdas with - them. + construction/assignment of the target callable. In addition, while + it is possible to use std::function in such a way that avoids most + of the overhead by making sure to only construct it with callables + of types that fit std::function's small object optimization, such + as function pointers and std::reference_wrapper callables, that is + quite inconvenient in practice, because restricting to + free-function callables would imply no state/capture/closure, which + we need in most cases, and std::reference_wrapper implies + remembering to use std::ref/std::cref where the callable is + constructed, with the added inconvenience that std::ref/std::cref + have deleted rvalue-ref overloads, meaning you can't use + unnamed/temporary lambdas with them. Note that because function_view is a non-owning view of a callable, care must be taken to ensure that the callable outlives the @@ -81,15 +115,16 @@ void iterate_over_foos (gdb::function_view<void (foo *)> callback) { - for (auto & : foos) - callback (&foo); + for (auto &foo : foos) + callback (&foo); } you can call it like this, passing a lambda as callback: - iterate_over_foos ([&] (foo *f) { - process_one_foo (f); - }); + iterate_over_foos ([&] (foo *f) + { + process_one_foo (f); + }); or like this, passing a function object as callback: @@ -97,15 +132,16 @@ { void operator() (foo *f) { - if (s->check ()) - process_one_foo (f); + if (s->check ()) + process_one_foo (f); } // some state state *s; }; - function_object matcher (mystate); + state mystate; + function_object matcher {&mystate}; iterate_over_foos (matcher); or like this, passing a function pointer as callback: diff --git a/gdb/unittests/function-view-selftests.c b/gdb/unittests/function-view-selftests.c index 8f73bc4..3e5369b 100644 --- a/gdb/unittests/function-view-selftests.c +++ b/gdb/unittests/function-view-selftests.c @@ -27,143 +27,144 @@ namespace selftests { namespace function_view { static int -add_one (int count) +plus_one_fn_int (int val) { - return ++count; + return ++val; } static short -add_one_short (short count) +plus_one_fn_short (short val) { - return ++count; + return ++val; } static int -call_callback (int count, gdb::function_view <int(int)> callback) +call_callback_int (int val, gdb::function_view <int (int)> callback) { - return callback (count); + return callback (val); } static void -call_callback_void (int count, gdb::function_view <void(int)> callback) +call_callback_void (int val, gdb::function_view <void (int)> callback) { - callback (count); + callback (val); } -struct plus_one_function_object +struct plus_one_int_func_obj { - int operator () (int count) + int operator () (int val) { - ++calls; - return ++count; + ++call_count; + return ++val; } - int calls = 0; + /* Number of times called. */ + int call_count = 0; }; static void run_tests () { /* A simple lambda. */ - auto plus_one = [] (int count) { return ++count; }; + auto plus_one_lambda = [] (int val) { return ++val; }; /* A function_view that references the lambda. */ - gdb::function_view<int (int)> plus_one_view (plus_one); + gdb::function_view<int (int)> plus_one_func_view (plus_one_lambda); /* Check calling the lambda directly. */ - SELF_CHECK (plus_one (0) == 1); - SELF_CHECK (plus_one (1) == 2); + SELF_CHECK (plus_one_lambda (0) == 1); + SELF_CHECK (plus_one_lambda (1) == 2); /* Check calling lambda via the view. */ - SELF_CHECK (plus_one_view (2) == 3); - SELF_CHECK (plus_one_view (3) == 4); + SELF_CHECK (plus_one_func_view (2) == 3); + SELF_CHECK (plus_one_func_view (3) == 4); /* Check calling a function that takes a function_view as argument, by value. Pass a lambda, making sure a function_view is properly constructed implicitly. */ - SELF_CHECK (call_callback (1, [] (int count) + SELF_CHECK (call_callback_int (1, [] (int val) { - return count + 2; + return val + 2; }) == 3); /* Same, passing a named/lvalue lambda. */ - SELF_CHECK (call_callback (1, plus_one) == 2); + SELF_CHECK (call_callback_int (1, plus_one_lambda) == 2); /* Same, passing named/lvalue function_view (should copy). */ - SELF_CHECK (call_callback (1, plus_one_view) == 2); + SELF_CHECK (call_callback_int (1, plus_one_func_view) == 2); /* Check constructing a function view over a function-object callable, and calling it. */ - plus_one_function_object func_obj; + plus_one_int_func_obj func_obj; SELF_CHECK (func_obj (0) == 1); - SELF_CHECK (call_callback (1, func_obj) == 2); + SELF_CHECK (call_callback_int (1, func_obj) == 2); /* Check that the callable was referenced, not copied. */ - SELF_CHECK (func_obj.calls == 2); + SELF_CHECK (func_obj.call_count == 2); - /* Check constructing a function_view over free-function callable, + /* Check constructing a function_view over a free-function callable, and calling it. */ - SELF_CHECK (call_callback (1, add_one) == 2); + SELF_CHECK (call_callback_int (1, plus_one_fn_int) == 2); /* Check calling a function with a compatible-but-not-exactly-the-same prototype. */ - SELF_CHECK (call_callback (1, [] (short count) -> short + SELF_CHECK (call_callback_int (1, [] (short val) -> short { - return count + 2; + return val + 2; }) == 3); /* Same, but passing a function pointer. */ - SELF_CHECK (call_callback (1, add_one_short) == 2); + SELF_CHECK (call_callback_int (1, plus_one_fn_short) == 2); /* Like std::function, a function_view that expects a void return can reference callables with non-void return type. The result is simply discarded. Check a lambda, function object and a function pointer. */ - call_callback_void (1, [] (int count) -> int + call_callback_void (1, [] (int val) -> int { - return count + 2; + return val + 2; }); call_callback_void (1, func_obj); - call_callback_void (1, add_one); + call_callback_void (1, plus_one_fn_int); /* Check that the main ctor doesn't hijack the copy ctor. */ - auto plus_one_view2 (plus_one_view); - auto plus_one_view3 (plus_one_view2); - static_assert (std::is_same<decltype (plus_one_view), - decltype (plus_one_view2)>::value, ""); - static_assert (std::is_same<decltype (plus_one_view), - decltype (plus_one_view3)>::value, ""); + auto plus_one_func_view2 (plus_one_func_view); + auto plus_one_func_view3 (plus_one_func_view2); + static_assert (std::is_same<decltype (plus_one_func_view), + decltype (plus_one_func_view2)>::value, ""); + static_assert (std::is_same<decltype (plus_one_func_view), + decltype (plus_one_func_view3)>::value, ""); - SELF_CHECK (plus_one_view3 (1) == 2); + SELF_CHECK (plus_one_func_view3 (1) == 2); /* Likewise, but propagate a NULL callable. If this calls the main function_view ctor instead of the copy ctor by mistake, then null_func_2 ends up non-NULL (because it'd instead reference null_func_1 as just another callable). */ - constexpr gdb::function_view<int(int)> null_func_1 = nullptr; - constexpr auto null_func_2 (null_func_1); + constexpr gdb::function_view<int (int)> null_func_view_1 = nullptr; + constexpr auto null_func_view_2 (null_func_view_1); /* While at it, check whether the function_view is bound using various forms, op==, op!= and op bool. */ /* op== */ - static_assert (null_func_2 == nullptr, ""); - static_assert (nullptr == null_func_2, ""); - static_assert (null_func_2 == NULL, ""); - static_assert (NULL == null_func_2, ""); + static_assert (null_func_view_2 == nullptr, ""); + static_assert (nullptr == null_func_view_2, ""); + static_assert (null_func_view_2 == NULL, ""); + static_assert (NULL == null_func_view_2, ""); /* op!= */ - static_assert (!(null_func_2 != nullptr), ""); - static_assert (!(nullptr != null_func_2), ""); - static_assert (!(null_func_2 != NULL), ""); - static_assert (!(NULL != null_func_2), ""); + static_assert (!(null_func_view_2 != nullptr), ""); + static_assert (!(nullptr != null_func_view_2), ""); + static_assert (!(null_func_view_2 != NULL), ""); + static_assert (!(NULL != null_func_view_2), ""); /* op bool */ - static_assert (!null_func_2, ""); + static_assert (!null_func_view_2, ""); /* Check the nullptr_t ctor. */ - constexpr gdb::function_view<int(int)> check_ctor_nullptr (nullptr); + constexpr gdb::function_view<int (int)> check_ctor_nullptr (nullptr); static_assert (!check_ctor_nullptr, ""); /* Check the nullptr_t op= */ - gdb::function_view<int(int)> check_op_eq_null (add_one); + gdb::function_view<int (int)> check_op_eq_null (plus_one_fn_int); SELF_CHECK (check_op_eq_null); check_op_eq_null = nullptr; SELF_CHECK (!check_op_eq_null); -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1.1 1/3] Introduce gdb::function_view 2017-02-22 17:40 ` Pedro Alves @ 2017-02-22 17:49 ` Pedro Alves 2017-02-22 22:12 ` Yao Qi 2017-02-22 22:23 ` Yao Qi 2017-02-22 18:02 ` [PATCH " Simon Marchi 1 sibling, 2 replies; 27+ messages in thread From: Pedro Alves @ 2017-02-22 17:49 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On 02/22/2017 05:40 PM, Pedro Alves wrote: > I'll send the updated (squashed) patch as a reply. Here is is. Thanks for the quick review! From 997f0e94cbd2d1cdb4bf7612df1de72ce1a6eb56 Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Wed, 22 Feb 2017 17:43:09 +0000 Subject: [PATCH] Introduce gdb::function_view This commit adds new function_view type. This type holds a a non-owning reference to a callable. It is meant to be used as callback type of functions, instead of using C-style pair of function pointer and 'void *data' arguments. function_view allows passing references to stateful function objects / lambdas w/ captures as callbacks efficiently, while function pointer + 'void *' does not. See the intro in the new function-view.h header for more. Unit tests included. I added a new gdb/unittests/ subdir this time, instead of putting the tests under gdb/. If this is agreed to be a good idea, some of the current selftests that exercise gdb/common/ things but live in gdb/ could move here (e.g., gdb/utils-selftests.c). gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS): New. (SFILES): Add $(SUBDIR_UNITTEST_SRCS). (COMMON_OBS): Add $(SUBDIR_UNITTEST_OBS). (%.o) <unittests/%.c>: New pattern. (INIT_FILES): Add $(SUBDIR_UNITTESTS_SRCS). * common/function-view.h: New file. * unittests/function-view-selftests.c: New file. --- gdb/Makefile.in | 24 ++- gdb/common/function-view.h | 355 ++++++++++++++++++++++++++++++++ gdb/unittests/function-view-selftests.c | 184 +++++++++++++++++ 3 files changed, 560 insertions(+), 3 deletions(-) create mode 100644 gdb/common/function-view.h create mode 100644 gdb/unittests/function-view-selftests.c diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 43253d3..a4cac36 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -523,6 +523,12 @@ SUBDIR_PYTHON_DEPS = SUBDIR_PYTHON_LDFLAGS = SUBDIR_PYTHON_CFLAGS = +SUBDIR_UNITTESTS_SRCS = \ + unittests/function-view-selftests.c + +SUBDIR_UNITTESTS_OBS = \ + function-view-selftests.o + # Opcodes currently live in one of two places. Either they are in the # opcode library, typically ../opcodes, or they are in a header file # in INCLUDE_DIR. @@ -1216,7 +1222,8 @@ SFILES = \ common/xml-utils.c \ mi/mi-common.c \ target/waitstatus.c \ - $(SUBDIR_GCC_COMPILE_SRCS) + $(SUBDIR_GCC_COMPILE_SRCS) \ + $(SUBDIR_UNITTEST_SRCS) LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c @@ -1800,7 +1807,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ xml-syscall.o \ xml-tdesc.o \ xml-utils.o \ - $(SUBDIR_GCC_COMPILE_OBS) + $(SUBDIR_GCC_COMPILE_OBS) \ + $(SUBDIR_UNITTESTS_OBS) TSOBS = inflow.o @@ -1909,6 +1917,10 @@ all: gdb$(EXEEXT) $(CONFIG_ALL) $(COMPILE) $< $(POSTCOMPILE) +%.o: ${srcdir}/unittests/%.c + $(COMPILE) $< + $(POSTCOMPILE) + # Specify an explicit rule for gdb/common/agent.c, to avoid a clash with the # object file generate by gdb/agent.c. common-agent.o: $(srcdir)/common/agent.c @@ -2124,7 +2136,13 @@ test-cp-name-parser$(EXEEXT): test-cp-name-parser.o $(LIBIBERTY) # duplicates. Files in the gdb/ directory can end up appearing in # COMMON_OBS (as a .o file) and CONFIG_SRCS (as a .c file). -INIT_FILES = $(COMMON_OBS) $(TSOBS) $(CONFIG_SRCS) $(SUBDIR_GCC_COMPILE_SRCS) +INIT_FILES = \ + $(COMMON_OBS) \ + $(TSOBS) \ + $(CONFIG_SRCS) \ + $(SUBDIR_GCC_COMPILE_SRCS) \ + $(SUBDIR_UNITTESTS_SRCS) + init.c: $(INIT_FILES) @echo Making init.c @rm -f init.c-tmp init.l-tmp diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h new file mode 100644 index 0000000..af2593f --- /dev/null +++ b/gdb/common/function-view.h @@ -0,0 +1,355 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifndef COMMON_FUNCTION_VIEW_H +#define COMMON_FUNCTION_VIEW_H + +/* function_view is a polymorphic type-erasing wrapper class that + encapsulates a non-owning reference to arbitrary callable objects. + + A way to put it is that function_view is to std::function like + std::string_view is to std::string. While std::function stores a + type-erased callable object internally, function_view holds a + type-erased reference to an external callable object. + + This is meant to be used as callback type of a function that: + + #1 - Takes a callback as parameter. + + #2 - Wants to support arbitrary callable objects as callback type + (e.g., stateful function objects, lambda closures, free + functions). + + #3 - Does not store the callback anywhere; instead the function + just calls the callback directly or forwards it to some + other function that calls it. + + #4 - Can't be, or we don't want it to be, a template function + with the callable type as template parameter. For example, + when the callback is a parameter of a virtual member + function, or when putting the function template in a header + would expose too much implementation detail. + + Note that the C-style "function pointer" + "void *data" callback + parameter idiom fails requirement #2 above. Please don't add new + uses of that idiom. I.e., something like this wouldn't work; + + typedef bool (iterate_over_foos_cb) (foo *f, void *user_data), + void iterate_over_foos (iterate_over_foos_cb *callback, void *user_data); + + foo *find_foo_by_type (int type) + { + foo *found = nullptr; + + iterate_over_foos ([&] (foo *f, void *data) + { + if (foo->type == type) + { + found = foo; + return true; // stop iterating + } + return false; // continue iterating + }, NULL); + + return found; + } + + The above wouldn't compile, because lambdas with captures can't be + implicitly converted to a function pointer (because a capture means + some context data must be passed to the lambda somehow). + + C++11 gave us std::function as type-erased wrapper around arbitrary + callables, however, std::function is not an ideal fit for transient + callbacks such as the use case above. For this use case, which is + quite pervasive, a function_view is a better choice, because while + while function_view is light and does not require any heap + allocation, std::function is a heavy-weight object with value + semantics that generally requires a heap allocation on + construction/assignment of the target callable. In addition, while + it is possible to use std::function in such a way that avoids most + of the overhead by making sure to only construct it with callables + of types that fit std::function's small object optimization, such + as function pointers and std::reference_wrapper callables, that is + quite inconvenient in practice, because restricting to + free-function callables would imply no state/capture/closure, which + we need in most cases, and std::reference_wrapper implies + remembering to use std::ref/std::cref where the callable is + constructed, with the added inconvenience that std::ref/std::cref + have deleted rvalue-ref overloads, meaning you can't use + unnamed/temporary lambdas with them. + + Note that because function_view is a non-owning view of a callable, + care must be taken to ensure that the callable outlives the + function_view that calls it. This is not really a problem for the + use case function_view is intended for, such as passing a temporary + function object / lambda to a function that accepts a callback, + because in those cases, the temporary is guaranteed to be live + until the called function returns. + + Calling a function_view with no associated target is undefined, + unlike with std::function, which throws bad_function_call. This is + by design, to avoid the otherwise necessary NULL check in + function_view::operator(). + + Since function_view objects are small (a pair of pointers), they + should generally be passed around by value. + + Usage: + + Given this function that accepts a callback: + + void + iterate_over_foos (gdb::function_view<void (foo *)> callback) + { + for (auto &foo : foos) + callback (&foo); + } + + you can call it like this, passing a lambda as callback: + + iterate_over_foos ([&] (foo *f) + { + process_one_foo (f); + }); + + or like this, passing a function object as callback: + + struct function_object + { + void operator() (foo *f) + { + if (s->check ()) + process_one_foo (f); + } + + // some state + state *s; + }; + + state mystate; + function_object matcher {&mystate}; + iterate_over_foos (matcher); + + or like this, passing a function pointer as callback: + + iterate_over_foos (process_one_foo); + + You can find unit tests covering the whole API in + unittests/function-view-selftests.c. */ + +namespace gdb { + +namespace traits { + /* A few trait helpers. */ + template<typename Predicate> + struct Not : public std::integral_constant<bool, !Predicate::value> + {}; + + template<typename...> + struct Or; + + template<> + struct Or<> : public std::false_type + {}; + + template<typename B1> + struct Or<B1> : public B1 + {}; + + template<typename B1, typename B2> + struct Or<B1, B2> + : public std::conditional<B1::value, B1, B2>::type + {}; + + template<typename B1,typename B2,typename B3, typename... Bn> + struct Or<B1, B2, B3, Bn...> + : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type + {}; +} /* namespace traits */ + +namespace fv_detail { +/* Bits shared by all function_view instantiations that do not depend + on the template parameters. */ + +/* Storage for the erased callable. This is a union in order to be + able to save both a function object (data) pointer or a function + pointer without triggering undefined behavior. */ +union erased_callable +{ + /* For function objects. */ + void *data; + + /* For function pointers. */ + void (*fn) (); +}; + +} /* namespace fv_detail */ + +/* Use partial specialization to get access to the callable's + signature. */ +template<class Signature> +struct function_view; + +template<typename Res, typename... Args> +class function_view<Res (Args...)> +{ + template<typename From, typename To> + using CompatibleReturnType + = traits::Or<std::is_void<To>, + std::is_same<From, To>, + std::is_convertible<From, To>>; + + /* True if Func can be called with Args, and the result, and the + result is convertible to Res, unless Res is void. */ + template<typename Callable, + typename Res2 = typename std::result_of<Callable &(Args...)>::type> + struct IsCompatibleCallable : CompatibleReturnType<Res2, Res> + {}; + + /* True if Callable is a function_view. Used to avoid hijacking the + copy ctor. */ + template <typename Callable> + struct IsFunctionView + : std::is_same<function_view, typename std::decay<Callable>::type> + {}; + + /* Helper to make SFINAE logic easier to read. */ + template<typename Condition> + using Requires = typename std::enable_if<Condition::value, void>::type; + + public: + + /* NULL by default. */ + constexpr function_view () noexcept + : m_erased_callable {}, + m_invoker {} + {} + + /* Default copy/assignment is fine. */ + function_view (const function_view &) = default; + function_view &operator= (const function_view &) = default; + + /* This is the main entry point. Use SFINAE to avoid hijacking the + copy constructor and to ensure that the target type is + compatible. */ + template + <typename Callable, + typename = Requires<traits::Not<IsFunctionView<Callable>>>, + typename = Requires<IsCompatibleCallable<Callable>>> + function_view (Callable &&callable) noexcept + { + bind (callable); + } + + /* Construct a NULL function_view. */ + constexpr function_view (std::nullptr_t) noexcept + : m_erased_callable {}, + m_invoker {} + {} + + /* Clear a function_view. */ + function_view &operator= (std::nullptr_t) noexcept + { + m_invoker = nullptr; + return *this; + } + + /* Return true if the wrapper has a target, false otherwise. Note + we check M_INVOKER instead of M_ERASED_CALLABLE because we don't + know which member of the union is active right now. */ + constexpr explicit operator bool () const noexcept + { return m_invoker != nullptr; } + + /* Call the callable. */ + Res operator () (Args... args) const + { return m_invoker (m_erased_callable, std::forward<Args> (args)...); } + + private: + + /* Bind this function_view to a compatible function object + reference. */ + template <typename Callable> + void bind (Callable &callable) noexcept + { + m_erased_callable.data = (void *) std::addressof (callable); + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) + noexcept (noexcept (callable (std::forward<Args> (args)...))) -> Res + { + auto &restored_callable = *static_cast<Callable *> (ecall.data); + /* The explicit cast to Res avoids a compile error when Res is + void and the callable returns non-void. */ + return (Res) restored_callable (std::forward<Args> (args)...); + }; + } + + /* Bind this function_view to a compatible function pointer. + + Making this a separate function allows avoiding one indirection, + by storing the function pointer directly in the storage, instead + of a pointer to pointer. erased_callable is then a union in + order to avoid storing a function pointer as a data pointer here, + which would be undefined. */ + template<class Res2, typename... Args2> + void bind (Res2 (*fn) (Args2...)) noexcept + { + m_erased_callable.fn = reinterpret_cast<void (*) ()> (fn); + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) + noexcept (noexcept (fn (std::forward<Args> (args)...))) -> Res + { + auto restored_fn = reinterpret_cast<Res2 (*) (Args2...)> (ecall.fn); + /* The explicit cast to Res avoids a compile error when Res is + void and the callable returns non-void. */ + return (Res) restored_fn (std::forward<Args> (args)...); + }; + } + + /* Storage for the erased callable. */ + fv_detail::erased_callable m_erased_callable; + + /* The invoker. This is set to a capture-less lambda by one of the + 'bind' overloads. The lambda restores the right type of the + callable (which is passed as first argument), and forwards the + args. */ + Res (*m_invoker) (fv_detail::erased_callable, Args...); +}; + +/* Allow comparison with NULL. Defer the work to the in-class + operator bool implementation. */ + +template<typename Res, typename... Args> +constexpr inline bool +operator== (const function_view<Res (Args...)> &f, std::nullptr_t) noexcept +{ return !static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator== (std::nullptr_t, const function_view<Res (Args...)> &f) noexcept +{ return !static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator!= (const function_view<Res (Args...)> &f, std::nullptr_t) noexcept +{ return static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator!= (std::nullptr_t, const function_view<Res (Args...)> &f) noexcept +{ return static_cast<bool> (f); } + +} /* namespace gdb */ + +#endif diff --git a/gdb/unittests/function-view-selftests.c b/gdb/unittests/function-view-selftests.c new file mode 100644 index 0000000..3e5369b --- /dev/null +++ b/gdb/unittests/function-view-selftests.c @@ -0,0 +1,184 @@ +/* Self tests for function_view for GDB, the GNU debugger. + + Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include "defs.h" +#include "selftest.h" +#include "common/function-view.h" + +#if GDB_SELF_TEST + +namespace selftests { +namespace function_view { + +static int +plus_one_fn_int (int val) +{ + return ++val; +} + +static short +plus_one_fn_short (short val) +{ + return ++val; +} + +static int +call_callback_int (int val, gdb::function_view <int (int)> callback) +{ + return callback (val); +} + +static void +call_callback_void (int val, gdb::function_view <void (int)> callback) +{ + callback (val); +} + +struct plus_one_int_func_obj +{ + int operator () (int val) + { + ++call_count; + return ++val; + } + + /* Number of times called. */ + int call_count = 0; +}; + +static void +run_tests () +{ + /* A simple lambda. */ + auto plus_one_lambda = [] (int val) { return ++val; }; + + /* A function_view that references the lambda. */ + gdb::function_view<int (int)> plus_one_func_view (plus_one_lambda); + + /* Check calling the lambda directly. */ + SELF_CHECK (plus_one_lambda (0) == 1); + SELF_CHECK (plus_one_lambda (1) == 2); + + /* Check calling lambda via the view. */ + SELF_CHECK (plus_one_func_view (2) == 3); + SELF_CHECK (plus_one_func_view (3) == 4); + + /* Check calling a function that takes a function_view as argument, + by value. Pass a lambda, making sure a function_view is properly + constructed implicitly. */ + SELF_CHECK (call_callback_int (1, [] (int val) + { + return val + 2; + }) == 3); + + /* Same, passing a named/lvalue lambda. */ + SELF_CHECK (call_callback_int (1, plus_one_lambda) == 2); + /* Same, passing named/lvalue function_view (should copy). */ + SELF_CHECK (call_callback_int (1, plus_one_func_view) == 2); + + /* Check constructing a function view over a function-object + callable, and calling it. */ + plus_one_int_func_obj func_obj; + SELF_CHECK (func_obj (0) == 1); + SELF_CHECK (call_callback_int (1, func_obj) == 2); + /* Check that the callable was referenced, not copied. */ + SELF_CHECK (func_obj.call_count == 2); + + /* Check constructing a function_view over a free-function callable, + and calling it. */ + SELF_CHECK (call_callback_int (1, plus_one_fn_int) == 2); + + /* Check calling a function with a + compatible-but-not-exactly-the-same prototype. */ + SELF_CHECK (call_callback_int (1, [] (short val) -> short + { + return val + 2; + }) == 3); + /* Same, but passing a function pointer. */ + SELF_CHECK (call_callback_int (1, plus_one_fn_short) == 2); + + /* Like std::function, a function_view that expects a void return + can reference callables with non-void return type. The result is + simply discarded. Check a lambda, function object and a function + pointer. */ + call_callback_void (1, [] (int val) -> int + { + return val + 2; + }); + call_callback_void (1, func_obj); + call_callback_void (1, plus_one_fn_int); + + /* Check that the main ctor doesn't hijack the copy ctor. */ + auto plus_one_func_view2 (plus_one_func_view); + auto plus_one_func_view3 (plus_one_func_view2); + static_assert (std::is_same<decltype (plus_one_func_view), + decltype (plus_one_func_view2)>::value, ""); + static_assert (std::is_same<decltype (plus_one_func_view), + decltype (plus_one_func_view3)>::value, ""); + + SELF_CHECK (plus_one_func_view3 (1) == 2); + + /* Likewise, but propagate a NULL callable. If this calls the main + function_view ctor instead of the copy ctor by mistake, then + null_func_2 ends up non-NULL (because it'd instead reference + null_func_1 as just another callable). */ + constexpr gdb::function_view<int (int)> null_func_view_1 = nullptr; + constexpr auto null_func_view_2 (null_func_view_1); + + /* While at it, check whether the function_view is bound using + various forms, op==, op!= and op bool. */ + + /* op== */ + static_assert (null_func_view_2 == nullptr, ""); + static_assert (nullptr == null_func_view_2, ""); + static_assert (null_func_view_2 == NULL, ""); + static_assert (NULL == null_func_view_2, ""); + + /* op!= */ + static_assert (!(null_func_view_2 != nullptr), ""); + static_assert (!(nullptr != null_func_view_2), ""); + static_assert (!(null_func_view_2 != NULL), ""); + static_assert (!(NULL != null_func_view_2), ""); + + /* op bool */ + static_assert (!null_func_view_2, ""); + + /* Check the nullptr_t ctor. */ + constexpr gdb::function_view<int (int)> check_ctor_nullptr (nullptr); + static_assert (!check_ctor_nullptr, ""); + + /* Check the nullptr_t op= */ + gdb::function_view<int (int)> check_op_eq_null (plus_one_fn_int); + SELF_CHECK (check_op_eq_null); + check_op_eq_null = nullptr; + SELF_CHECK (!check_op_eq_null); +} + +} /* namespace function_view */ +} /* namespace selftests */ + +#endif + +void +_initialize_function_view_selftests () +{ +#if GDB_SELF_TEST + register_self_test (selftests::function_view::run_tests); +#endif +} -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.1 1/3] Introduce gdb::function_view 2017-02-22 17:49 ` [PATCH v1.1 " Pedro Alves @ 2017-02-22 22:12 ` Yao Qi 2017-02-23 14:49 ` Pedro Alves 2017-02-22 22:23 ` Yao Qi 1 sibling, 1 reply; 27+ messages in thread From: Yao Qi @ 2017-02-22 22:12 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches On 17-02-22 17:49:22, Pedro Alves wrote: > On 02/22/2017 05:40 PM, Pedro Alves wrote: > > > I'll send the updated (squashed) patch as a reply. > > Here is is. Thanks for the quick review! > > From 997f0e94cbd2d1cdb4bf7612df1de72ce1a6eb56 Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Wed, 22 Feb 2017 17:43:09 +0000 > Subject: [PATCH] Introduce gdb::function_view > > This commit adds new function_view type. This type holds a a Double "a" at the end. > non-owning reference to a callable. It is meant to be used as > callback type of functions, instead of using C-style pair of function > pointer and 'void *data' arguments. function_view allows passing > references to stateful function objects / lambdas w/ captures as > callbacks efficiently, while function pointer + 'void *' does not. > > See the intro in the new function-view.h header for more. > > Unit tests included. I added a new gdb/unittests/ subdir this time, > instead of putting the tests under gdb/. If this is agreed to be a > good idea, some of the current selftests that exercise gdb/common/ > things but live in gdb/ could move here (e.g., gdb/utils-selftests.c). I wanted to add gdb/unittests for a while, but didn't have a chance to do so. Yes, it is a good idea to me. How is GDB unit tests, like disasm-selftests.c? Do you want to move it to gdb/unittests/? > + C++11 gave us std::function as type-erased wrapper around arbitrary > + callables, however, std::function is not an ideal fit for transient > + callbacks such as the use case above. For this use case, which is > + quite pervasive, a function_view is a better choice, because while > + while function_view is light and does not require any heap Double "while"s. > +namespace gdb { > + This is a new namespace in gdb source. What is the rule of using this namespace? -- Yao ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.1 1/3] Introduce gdb::function_view 2017-02-22 22:12 ` Yao Qi @ 2017-02-23 14:49 ` Pedro Alves 2017-02-23 15:11 ` Yao Qi 0 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-23 14:49 UTC (permalink / raw) To: Yao Qi; +Cc: Simon Marchi, gdb-patches On 02/22/2017 10:11 PM, Yao Qi wrote: > Double "a" at the end. > > Double "while"s. Thanks, fixed. > >> +namespace gdb { >> + > > This is a new namespace in gdb source. What is the rule of using this > namespace? I don't have a hard rule predetermined. I originally put the C++03 unique_ptr shim under the gdb namespace to make it clear at the call sites that that was not the std type, but gdb's replacement. I.e., gdb::unique_ptr vs std::unique_ptr. Since then, gdb::unique_ptr is gone, but gdb::unique_xmalloc_ptr remains, and we've been putting these generic library-like utilities under the namespace: common/function-view.h:154:namespace gdb { common/function-view.h:353:} /* namespace gdb */ common/gdb_unlinker.h:23:namespace gdb common/gdb_unique_ptr.h:25:namespace gdb common/gdb_unique_ptr.h:43:} /* namespace gdb */ common/gdb_optional.h:23:namespace gdb common/gdb_ref_ptr.h:25:namespace gdb I think putting these new things in _some_ namespace is the right thing to do. gdb is just the no-brainer namespace name. IMO, _all_ of GDB should be under "namespace gdb". Then these utilities would either be put in "namespace gdb" too, or in a "namespace gtl", for "gdb template library" or something like that. Or we could put them under "gtl" already. I have a series that put the whole of gdb under namespace gdb: https://github.com/palves/gdb/commits/palves/cxx-gdb-namespace I wrote that while experimenting with ways to use gnulib's C++ namespace mode, in order to avoid the "#define open rpl_open" defines gnulib does... I nowadays believe the put-gdb-in-a-namespace approach is the best one. However, when we put gdb under a namespace, debugging GDB itself becomes a bit harder, since you then have to do "b gdb::some_function" instead of "b some_function". This is just dogfooding pain, of course, users run into this too... That's what led to me to work on this branch: https://github.com/palves/gdb/commits/palves/cp-linespec-2 with that, "b some_function" sets breakpoints on all functions/methods named some_function in all namespaces. And it was testing that branch with --target_board=dwarf4-gdb-index that led to the bug fixed by this series... It's a never ending story... :-) Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.1 1/3] Introduce gdb::function_view 2017-02-23 14:49 ` Pedro Alves @ 2017-02-23 15:11 ` Yao Qi 2017-02-23 15:20 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Yao Qi @ 2017-02-23 15:11 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches Pedro Alves <palves@redhat.com> writes: > I think putting these new things in _some_ namespace is the > right thing to do. gdb is just the no-brainer namespace name. Yes, that right. > > IMO, _all_ of GDB should be under "namespace gdb". Then these utilities > would either be put in "namespace gdb" too, or in a "namespace gtl", > for "gdb template library" or something like that. Or we could > put them under "gtl" already. IMO, "gdb template library" or "gtl" is project name, but it doesn't fit well as a namespace name. I like "gdb::utils" :). -- Yao (齐尧) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.1 1/3] Introduce gdb::function_view 2017-02-23 15:11 ` Yao Qi @ 2017-02-23 15:20 ` Pedro Alves 2017-02-23 15:34 ` Yao Qi 0 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-23 15:20 UTC (permalink / raw) To: Yao Qi; +Cc: Simon Marchi, gdb-patches On 02/23/2017 03:11 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> I think putting these new things in _some_ namespace is the >> right thing to do. gdb is just the no-brainer namespace name. > > Yes, that right. > >> >> IMO, _all_ of GDB should be under "namespace gdb". Then these utilities >> would either be put in "namespace gdb" too, or in a "namespace gtl", >> for "gdb template library" or something like that. Or we could >> put them under "gtl" already. > > IMO, "gdb template library" or "gtl" is project name, but it doesn't fit > well as a namespace name. I like "gdb::utils" :). "gdb::utils::function_view" is maybe a bit too long for code that is going to end up used a lot, though. "gdb::function_view" or "gtl::function_view" also has the "advantage" that replacing "gdb" with "std" at some point does not require reindenting the code. (e.g., gdb::optional -> std::optional when we get to C++17). 1/2 :-) I think the easiest is to keep using "gdb" for now, and revisit if/when we move everything under gdb. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.1 1/3] Introduce gdb::function_view 2017-02-23 15:20 ` Pedro Alves @ 2017-02-23 15:34 ` Yao Qi 0 siblings, 0 replies; 27+ messages in thread From: Yao Qi @ 2017-02-23 15:34 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches On Thu, Feb 23, 2017 at 3:20 PM, Pedro Alves <palves@redhat.com> wrote: > > I think the easiest is to keep using "gdb" for now, and revisit > if/when we move everything under gdb. > That is fine by me. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.1 1/3] Introduce gdb::function_view 2017-02-22 17:49 ` [PATCH v1.1 " Pedro Alves 2017-02-22 22:12 ` Yao Qi @ 2017-02-22 22:23 ` Yao Qi 2017-02-23 14:50 ` [PATCH v1.2 " Pedro Alves 1 sibling, 1 reply; 27+ messages in thread From: Yao Qi @ 2017-02-22 22:23 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches On 17-02-22 17:49:22, Pedro Alves wrote: > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > index 43253d3..a4cac36 100644 > --- a/gdb/Makefile.in > +++ b/gdb/Makefile.in > @@ -523,6 +523,12 @@ SUBDIR_PYTHON_DEPS = > SUBDIR_PYTHON_LDFLAGS = > SUBDIR_PYTHON_CFLAGS = > > +SUBDIR_UNITTESTS_SRCS = \ > + unittests/function-view-selftests.c > + > +SUBDIR_UNITTESTS_OBS = \ > + function-view-selftests.o > + > # Opcodes currently live in one of two places. Either they are in the > # opcode library, typically ../opcodes, or they are in a header file > # in INCLUDE_DIR. > @@ -1216,7 +1222,8 @@ SFILES = \ > common/xml-utils.c \ > mi/mi-common.c \ > target/waitstatus.c \ > - $(SUBDIR_GCC_COMPILE_SRCS) > + $(SUBDIR_GCC_COMPILE_SRCS) \ > + $(SUBDIR_UNITTEST_SRCS) > > LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c > > @@ -1800,7 +1807,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ > xml-syscall.o \ > xml-tdesc.o \ > xml-utils.o \ > - $(SUBDIR_GCC_COMPILE_OBS) > + $(SUBDIR_GCC_COMPILE_OBS) \ > + $(SUBDIR_UNITTESTS_OBS) > Can we add SUBDIR_UNITESTS_OBS to CONFIG_OBS (in configure.ac) if GDB_SELF_TEST is defined? > TSOBS = inflow.o > > @@ -1909,6 +1917,10 @@ all: gdb$(EXEEXT) $(CONFIG_ALL) > $(COMPILE) $< > $(POSTCOMPILE) > > +%.o: ${srcdir}/unittests/%.c > + $(COMPILE) $< > + $(POSTCOMPILE) > + > # Specify an explicit rule for gdb/common/agent.c, to avoid a clash with the > # object file generate by gdb/agent.c. > common-agent.o: $(srcdir)/common/agent.c > @@ -2124,7 +2136,13 @@ test-cp-name-parser$(EXEEXT): test-cp-name-parser.o $(LIBIBERTY) > # duplicates. Files in the gdb/ directory can end up appearing in > # COMMON_OBS (as a .o file) and CONFIG_SRCS (as a .c file). > > -INIT_FILES = $(COMMON_OBS) $(TSOBS) $(CONFIG_SRCS) $(SUBDIR_GCC_COMPILE_SRCS) > +INIT_FILES = \ > + $(COMMON_OBS) \ > + $(TSOBS) \ > + $(CONFIG_SRCS) \ > + $(SUBDIR_GCC_COMPILE_SRCS) \ > + $(SUBDIR_UNITTESTS_SRCS) > + If so, SUBDIR_UNITESTS_SRCS is added to CONFIG_SRCS, we don't need this change. > diff --git a/gdb/unittests/function-view-selftests.c b/gdb/unittests/function-view-selftests.c > new file mode 100644 > index 0000000..3e5369b > --- /dev/null > +++ b/gdb/unittests/function-view-selftests.c > @@ -0,0 +1,184 @@ > +/* Self tests for function_view for GDB, the GNU debugger. > + > + Copyright (C) 2017 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. */ > + > +#include "defs.h" > +#include "selftest.h" > +#include "common/function-view.h" > + > +#if GDB_SELF_TEST If gdb/unittests/*.c are compiled when GDB_SELF_TEST is defined, we don't need to check GDB_SELF_TEST here. > + > +namespace selftests { > +namespace function_view { > + > + > +} /* namespace function_view */ > +} /* namespace selftests */ > + > +#endif > + > +void > +_initialize_function_view_selftests () > +{ > +#if GDB_SELF_TEST This is not needed as well. > + register_self_test (selftests::function_view::run_tests); > +#endif > +} > -- > 2.5.5 > > -- Yao ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v1.2 1/3] Introduce gdb::function_view 2017-02-22 22:23 ` Yao Qi @ 2017-02-23 14:50 ` Pedro Alves 2017-02-23 14:58 ` Yao Qi 0 siblings, 1 reply; 27+ messages in thread From: Pedro Alves @ 2017-02-23 14:50 UTC (permalink / raw) To: Yao Qi; +Cc: Simon Marchi, gdb-patches On 02/22/2017 10:23 PM, Yao Qi wrote: > On 17-02-22 17:49:22, Pedro Alves wrote: >> LINTFILES = $(SFILES) $(YYFILES) $(CONFIG_SRCS) init.c >> >> @@ -1800,7 +1807,8 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \ >> xml-syscall.o \ >> xml-tdesc.o \ >> xml-utils.o \ >> - $(SUBDIR_GCC_COMPILE_OBS) >> + $(SUBDIR_GCC_COMPILE_OBS) \ >> + $(SUBDIR_UNITTESTS_OBS) >> > > Can we add SUBDIR_UNITESTS_OBS to CONFIG_OBS (in configure.ac) if > GDB_SELF_TEST is defined? Good idea, this works nicely. Here's the updated patch. From 735124ed86a210dd4cda290258f7c3bbe076fa0e Mon Sep 17 00:00:00 2001 From: Pedro Alves <palves@redhat.com> Date: Thu, 23 Feb 2017 14:11:17 +0000 Subject: [PATCH] Introduce gdb::function_view This commit adds a new function_view type. This type holds a non-owning reference to a callable. It is meant to be used as callback type of functions, instead of using the C-style pair of function pointer and 'void *data' arguments. function_view allows passing references to stateful function objects / lambdas with captures as callbacks efficiently, while function pointer + 'void *' does not. See the intro in the new function-view.h header for more. Unit tests included, put into a new gdb/unittests/ subdir. gdb/ChangeLog: yyyy-mm-dd Pedro Alves <palves@redhat.com> * Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS): New. (SFILES): Add $(SUBDIR_UNITTEST_SRCS). (COMMON_OBS): Add $(SUBDIR_UNITTEST_OBS). (%.o) <unittests/%.c>: New pattern. (INIT_FILES): Add $(SUBDIR_UNITTESTS_SRCS). * common/function-view.h: New file. * unittests/function-view-selftests.c: New file. --- gdb/Makefile.in | 10 + gdb/common/function-view.h | 355 ++++++++++++++++++++++++++++++++ gdb/configure | 2 + gdb/configure.ac | 2 + gdb/unittests/function-view-selftests.c | 178 ++++++++++++++++ 5 files changed, 547 insertions(+) create mode 100644 gdb/common/function-view.h create mode 100644 gdb/unittests/function-view-selftests.c diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 43253d3..268c2c6 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -523,6 +523,12 @@ SUBDIR_PYTHON_DEPS = SUBDIR_PYTHON_LDFLAGS = SUBDIR_PYTHON_CFLAGS = +SUBDIR_UNITTESTS_SRCS = \ + unittests/function-view-selftests.c + +SUBDIR_UNITTESTS_OBS = \ + function-view-selftests.o + # Opcodes currently live in one of two places. Either they are in the # opcode library, typically ../opcodes, or they are in a header file # in INCLUDE_DIR. @@ -1909,6 +1915,10 @@ all: gdb$(EXEEXT) $(CONFIG_ALL) $(COMPILE) $< $(POSTCOMPILE) +%.o: ${srcdir}/unittests/%.c + $(COMPILE) $< + $(POSTCOMPILE) + # Specify an explicit rule for gdb/common/agent.c, to avoid a clash with the # object file generate by gdb/agent.c. common-agent.o: $(srcdir)/common/agent.c diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h new file mode 100644 index 0000000..66a691b --- /dev/null +++ b/gdb/common/function-view.h @@ -0,0 +1,355 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifndef COMMON_FUNCTION_VIEW_H +#define COMMON_FUNCTION_VIEW_H + +/* function_view is a polymorphic type-erasing wrapper class that + encapsulates a non-owning reference to arbitrary callable objects. + + A way to put it is that function_view is to std::function like + std::string_view is to std::string. While std::function stores a + type-erased callable object internally, function_view holds a + type-erased reference to an external callable object. + + This is meant to be used as callback type of a function that: + + #1 - Takes a callback as parameter. + + #2 - Wants to support arbitrary callable objects as callback type + (e.g., stateful function objects, lambda closures, free + functions). + + #3 - Does not store the callback anywhere; instead the function + just calls the callback directly or forwards it to some + other function that calls it. + + #4 - Can't be, or we don't want it to be, a template function + with the callable type as template parameter. For example, + when the callback is a parameter of a virtual member + function, or when putting the function template in a header + would expose too much implementation detail. + + Note that the C-style "function pointer" + "void *data" callback + parameter idiom fails requirement #2 above. Please don't add new + uses of that idiom. I.e., something like this wouldn't work; + + typedef bool (iterate_over_foos_cb) (foo *f, void *user_data), + void iterate_over_foos (iterate_over_foos_cb *callback, void *user_data); + + foo *find_foo_by_type (int type) + { + foo *found = nullptr; + + iterate_over_foos ([&] (foo *f, void *data) + { + if (foo->type == type) + { + found = foo; + return true; // stop iterating + } + return false; // continue iterating + }, NULL); + + return found; + } + + The above wouldn't compile, because lambdas with captures can't be + implicitly converted to a function pointer (because a capture means + some context data must be passed to the lambda somehow). + + C++11 gave us std::function as type-erased wrapper around arbitrary + callables, however, std::function is not an ideal fit for transient + callbacks such as the use case above. For this use case, which is + quite pervasive, a function_view is a better choice, because while + function_view is light and does not require any heap allocation, + std::function is a heavy-weight object with value semantics that + generally requires a heap allocation on construction/assignment of + the target callable. In addition, while it is possible to use + std::function in such a way that avoids most of the overhead by + making sure to only construct it with callables of types that fit + std::function's small object optimization, such as function + pointers and std::reference_wrapper callables, that is quite + inconvenient in practice, because restricting to free-function + callables would imply no state/capture/closure, which we need in + most cases, and std::reference_wrapper implies remembering to use + std::ref/std::cref where the callable is constructed, with the + added inconvenience that std::ref/std::cref have deleted rvalue-ref + overloads, meaning you can't use unnamed/temporary lambdas with + them. + + Note that because function_view is a non-owning view of a callable, + care must be taken to ensure that the callable outlives the + function_view that calls it. This is not really a problem for the + use case function_view is intended for, such as passing a temporary + function object / lambda to a function that accepts a callback, + because in those cases, the temporary is guaranteed to be live + until the called function returns. + + Calling a function_view with no associated target is undefined, + unlike with std::function, which throws std::bad_function_call. + This is by design, to avoid the otherwise necessary NULL check in + function_view::operator(). + + Since function_view objects are small (a pair of pointers), they + should generally be passed around by value. + + Usage: + + Given this function that accepts a callback: + + void + iterate_over_foos (gdb::function_view<void (foo *)> callback) + { + for (auto &foo : foos) + callback (&foo); + } + + you can call it like this, passing a lambda as callback: + + iterate_over_foos ([&] (foo *f) + { + process_one_foo (f); + }); + + or like this, passing a function object as callback: + + struct function_object + { + void operator() (foo *f) + { + if (s->check ()) + process_one_foo (f); + } + + // some state + state *s; + }; + + state mystate; + function_object matcher {&mystate}; + iterate_over_foos (matcher); + + or like this, passing a function pointer as callback: + + iterate_over_foos (process_one_foo); + + You can find unit tests covering the whole API in + unittests/function-view-selftests.c. */ + +namespace gdb { + +namespace traits { + /* A few trait helpers. */ + template<typename Predicate> + struct Not : public std::integral_constant<bool, !Predicate::value> + {}; + + template<typename...> + struct Or; + + template<> + struct Or<> : public std::false_type + {}; + + template<typename B1> + struct Or<B1> : public B1 + {}; + + template<typename B1, typename B2> + struct Or<B1, B2> + : public std::conditional<B1::value, B1, B2>::type + {}; + + template<typename B1,typename B2,typename B3, typename... Bn> + struct Or<B1, B2, B3, Bn...> + : public std::conditional<B1::value, B1, Or<B2, B3, Bn...>>::type + {}; +} /* namespace traits */ + +namespace fv_detail { +/* Bits shared by all function_view instantiations that do not depend + on the template parameters. */ + +/* Storage for the erased callable. This is a union in order to be + able to save both a function object (data) pointer or a function + pointer without triggering undefined behavior. */ +union erased_callable +{ + /* For function objects. */ + void *data; + + /* For function pointers. */ + void (*fn) (); +}; + +} /* namespace fv_detail */ + +/* Use partial specialization to get access to the callable's + signature. */ +template<class Signature> +struct function_view; + +template<typename Res, typename... Args> +class function_view<Res (Args...)> +{ + template<typename From, typename To> + using CompatibleReturnType + = traits::Or<std::is_void<To>, + std::is_same<From, To>, + std::is_convertible<From, To>>; + + /* True if Func can be called with Args, and either the result is + Res, convertible to Res or Res is void. */ + template<typename Callable, + typename Res2 = typename std::result_of<Callable &(Args...)>::type> + struct IsCompatibleCallable : CompatibleReturnType<Res2, Res> + {}; + + /* True if Callable is a function_view. Used to avoid hijacking the + copy ctor. */ + template <typename Callable> + struct IsFunctionView + : std::is_same<function_view, typename std::decay<Callable>::type> + {}; + + /* Helper to make SFINAE logic easier to read. */ + template<typename Condition> + using Requires = typename std::enable_if<Condition::value, void>::type; + + public: + + /* NULL by default. */ + constexpr function_view () noexcept + : m_erased_callable {}, + m_invoker {} + {} + + /* Default copy/assignment is fine. */ + function_view (const function_view &) = default; + function_view &operator= (const function_view &) = default; + + /* This is the main entry point. Use SFINAE to avoid hijacking the + copy constructor and to ensure that the target type is + compatible. */ + template + <typename Callable, + typename = Requires<traits::Not<IsFunctionView<Callable>>>, + typename = Requires<IsCompatibleCallable<Callable>>> + function_view (Callable &&callable) noexcept + { + bind (callable); + } + + /* Construct a NULL function_view. */ + constexpr function_view (std::nullptr_t) noexcept + : m_erased_callable {}, + m_invoker {} + {} + + /* Clear a function_view. */ + function_view &operator= (std::nullptr_t) noexcept + { + m_invoker = nullptr; + return *this; + } + + /* Return true if the wrapper has a target, false otherwise. Note + we check M_INVOKER instead of M_ERASED_CALLABLE because we don't + know which member of the union is active right now. */ + constexpr explicit operator bool () const noexcept + { return m_invoker != nullptr; } + + /* Call the callable. */ + Res operator () (Args... args) const + { return m_invoker (m_erased_callable, std::forward<Args> (args)...); } + + private: + + /* Bind this function_view to a compatible function object + reference. */ + template <typename Callable> + void bind (Callable &callable) noexcept + { + m_erased_callable.data = (void *) std::addressof (callable); + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) + noexcept (noexcept (callable (std::forward<Args> (args)...))) -> Res + { + auto &restored_callable = *static_cast<Callable *> (ecall.data); + /* The explicit cast to Res avoids a compile error when Res is + void and the callable returns non-void. */ + return (Res) restored_callable (std::forward<Args> (args)...); + }; + } + + /* Bind this function_view to a compatible function pointer. + + Making this a separate function allows avoiding one indirection, + by storing the function pointer directly in the storage, instead + of a pointer to pointer. erased_callable is then a union in + order to avoid storing a function pointer as a data pointer here, + which would be undefined. */ + template<class Res2, typename... Args2> + void bind (Res2 (*fn) (Args2...)) noexcept + { + m_erased_callable.fn = reinterpret_cast<void (*) ()> (fn); + m_invoker = [] (fv_detail::erased_callable ecall, Args... args) + noexcept (noexcept (fn (std::forward<Args> (args)...))) -> Res + { + auto restored_fn = reinterpret_cast<Res2 (*) (Args2...)> (ecall.fn); + /* The explicit cast to Res avoids a compile error when Res is + void and the callable returns non-void. */ + return (Res) restored_fn (std::forward<Args> (args)...); + }; + } + + /* Storage for the erased callable. */ + fv_detail::erased_callable m_erased_callable; + + /* The invoker. This is set to a capture-less lambda by one of the + 'bind' overloads. The lambda restores the right type of the + callable (which is passed as first argument), and forwards the + args. */ + Res (*m_invoker) (fv_detail::erased_callable, Args...); +}; + +/* Allow comparison with NULL. Defer the work to the in-class + operator bool implementation. */ + +template<typename Res, typename... Args> +constexpr inline bool +operator== (const function_view<Res (Args...)> &f, std::nullptr_t) noexcept +{ return !static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator== (std::nullptr_t, const function_view<Res (Args...)> &f) noexcept +{ return !static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator!= (const function_view<Res (Args...)> &f, std::nullptr_t) noexcept +{ return static_cast<bool> (f); } + +template<typename Res, typename... Args> +constexpr inline bool +operator!= (std::nullptr_t, const function_view<Res (Args...)> &f) noexcept +{ return static_cast<bool> (f); } + +} /* namespace gdb */ + +#endif diff --git a/gdb/configure b/gdb/configure index 6df88d9..7fbae9f 100755 --- a/gdb/configure +++ b/gdb/configure @@ -17410,6 +17410,8 @@ if $development; then $as_echo "#define GDB_SELF_TEST 1" >>confdefs.h + CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_UNITTESTS_OBS)" + CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_UNITTESTS_SRCS)" fi diff --git a/gdb/configure.ac b/gdb/configure.ac index d2d854d..50f6f59 100644 --- a/gdb/configure.ac +++ b/gdb/configure.ac @@ -2350,6 +2350,8 @@ AC_DEFINE(GDB_DEFAULT_HOST_CHARSET, "UTF-8", if $development; then AC_DEFINE(GDB_SELF_TEST, 1, [Define if self-testing features should be enabled]) + CONFIG_OBS="$CONFIG_OBS \$(SUBDIR_UNITTESTS_OBS)" + CONFIG_SRCS="$CONFIG_SRCS \$(SUBDIR_UNITTESTS_SRCS)" fi GDB_AC_TRANSFORM([gdb], [GDB_TRANSFORM_NAME]) diff --git a/gdb/unittests/function-view-selftests.c b/gdb/unittests/function-view-selftests.c new file mode 100644 index 0000000..310f2ad --- /dev/null +++ b/gdb/unittests/function-view-selftests.c @@ -0,0 +1,178 @@ +/* Self tests for function_view for GDB, the GNU debugger. + + Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include "defs.h" +#include "selftest.h" +#include "common/function-view.h" + +namespace selftests { +namespace function_view { + +static int +plus_one_fn_int (int val) +{ + return ++val; +} + +static short +plus_one_fn_short (short val) +{ + return ++val; +} + +static int +call_callback_int (int val, gdb::function_view <int (int)> callback) +{ + return callback (val); +} + +static void +call_callback_void (int val, gdb::function_view <void (int)> callback) +{ + callback (val); +} + +struct plus_one_int_func_obj +{ + int operator () (int val) + { + ++call_count; + return ++val; + } + + /* Number of times called. */ + int call_count = 0; +}; + +static void +run_tests () +{ + /* A simple lambda. */ + auto plus_one_lambda = [] (int val) { return ++val; }; + + /* A function_view that references the lambda. */ + gdb::function_view<int (int)> plus_one_func_view (plus_one_lambda); + + /* Check calling the lambda directly. */ + SELF_CHECK (plus_one_lambda (0) == 1); + SELF_CHECK (plus_one_lambda (1) == 2); + + /* Check calling lambda via the view. */ + SELF_CHECK (plus_one_func_view (2) == 3); + SELF_CHECK (plus_one_func_view (3) == 4); + + /* Check calling a function that takes a function_view as argument, + by value. Pass a lambda, making sure a function_view is properly + constructed implicitly. */ + SELF_CHECK (call_callback_int (1, [] (int val) + { + return val + 2; + }) == 3); + + /* Same, passing a named/lvalue lambda. */ + SELF_CHECK (call_callback_int (1, plus_one_lambda) == 2); + /* Same, passing a named/lvalue function_view (should copy). */ + SELF_CHECK (call_callback_int (1, plus_one_func_view) == 2); + + /* Check constructing a function view over a function-object + callable, and calling it. */ + plus_one_int_func_obj func_obj; + SELF_CHECK (func_obj (0) == 1); + SELF_CHECK (call_callback_int (1, func_obj) == 2); + /* Check that the callable was referenced, not copied. */ + SELF_CHECK (func_obj.call_count == 2); + + /* Check constructing a function_view over a free-function callable, + and calling it. */ + SELF_CHECK (call_callback_int (1, plus_one_fn_int) == 2); + + /* Check calling a function with a + compatible-but-not-exactly-the-same prototype. */ + SELF_CHECK (call_callback_int (1, [] (short val) -> short + { + return val + 2; + }) == 3); + /* Same, but passing a function pointer. */ + SELF_CHECK (call_callback_int (1, plus_one_fn_short) == 2); + + /* Like std::function, a function_view that expects a void return + can reference callables with non-void return type. The result is + simply discarded. Check a lambda, function object and a function + pointer. */ + call_callback_void (1, [] (int val) -> int + { + return val + 2; + }); + call_callback_void (1, func_obj); + call_callback_void (1, plus_one_fn_int); + + /* Check that the main ctor doesn't hijack the copy ctor. */ + auto plus_one_func_view2 (plus_one_func_view); + auto plus_one_func_view3 (plus_one_func_view2); + static_assert (std::is_same<decltype (plus_one_func_view), + decltype (plus_one_func_view2)>::value, ""); + static_assert (std::is_same<decltype (plus_one_func_view), + decltype (plus_one_func_view3)>::value, ""); + + SELF_CHECK (plus_one_func_view3 (1) == 2); + + /* Likewise, but propagate a NULL callable. If this calls the main + function_view ctor instead of the copy ctor by mistake, then + null_func_2 ends up non-NULL (because it'd instead reference + null_func_1 as just another callable). */ + constexpr gdb::function_view<int (int)> null_func_view_1 = nullptr; + constexpr auto null_func_view_2 (null_func_view_1); + + /* While at it, check whether the function_view is bound using + various forms, op==, op!= and op bool. */ + + /* op== */ + static_assert (null_func_view_2 == nullptr, ""); + static_assert (nullptr == null_func_view_2, ""); + static_assert (null_func_view_2 == NULL, ""); + static_assert (NULL == null_func_view_2, ""); + + /* op!= */ + static_assert (!(null_func_view_2 != nullptr), ""); + static_assert (!(nullptr != null_func_view_2), ""); + static_assert (!(null_func_view_2 != NULL), ""); + static_assert (!(NULL != null_func_view_2), ""); + + /* op bool */ + static_assert (!null_func_view_2, ""); + + /* Check the nullptr_t ctor. */ + constexpr gdb::function_view<int (int)> check_ctor_nullptr (nullptr); + static_assert (!check_ctor_nullptr, ""); + + /* Check the nullptr_t op= */ + gdb::function_view<int (int)> check_op_eq_null (plus_one_fn_int); + SELF_CHECK (check_op_eq_null); + check_op_eq_null = nullptr; + SELF_CHECK (!check_op_eq_null); +} + +} /* namespace function_view */ +} /* namespace selftests */ + +void +_initialize_function_view_selftests () +{ + register_self_test (selftests::function_view::run_tests); +} -- 2.5.5 ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.2 1/3] Introduce gdb::function_view 2017-02-23 14:50 ` [PATCH v1.2 " Pedro Alves @ 2017-02-23 14:58 ` Yao Qi 2017-02-23 14:59 ` Pedro Alves 0 siblings, 1 reply; 27+ messages in thread From: Yao Qi @ 2017-02-23 14:58 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches Pedro Alves <palves@redhat.com> writes: > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves <palves@redhat.com> > > * Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS): New. > (SFILES): Add $(SUBDIR_UNITTEST_SRCS). > (COMMON_OBS): Add $(SUBDIR_UNITTEST_OBS). > (%.o) <unittests/%.c>: New pattern. > (INIT_FILES): Add $(SUBDIR_UNITTESTS_SRCS). > * common/function-view.h: New file. > * unittests/function-view-selftests.c: New file. Patch is good to me. Nit, CL entry on configure.ac is missing. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v1.2 1/3] Introduce gdb::function_view 2017-02-23 14:58 ` Yao Qi @ 2017-02-23 14:59 ` Pedro Alves 0 siblings, 0 replies; 27+ messages in thread From: Pedro Alves @ 2017-02-23 14:59 UTC (permalink / raw) To: Yao Qi; +Cc: Simon Marchi, gdb-patches On 02/23/2017 02:58 PM, Yao Qi wrote: > Pedro Alves <palves@redhat.com> writes: > >> gdb/ChangeLog: >> yyyy-mm-dd Pedro Alves <palves@redhat.com> >> >> * Makefile.in (SUBDIR_UNITTESTS_SRCS, SUBDIR_UNITTESTS_OBS): New. >> (SFILES): Add $(SUBDIR_UNITTEST_SRCS). >> (COMMON_OBS): Add $(SUBDIR_UNITTEST_OBS). >> (%.o) <unittests/%.c>: New pattern. >> (INIT_FILES): Add $(SUBDIR_UNITTESTS_SRCS). >> * common/function-view.h: New file. >> * unittests/function-view-selftests.c: New file. > > Patch is good to me. Nit, CL entry on configure.ac is missing. Oh, I completely forgot to update the CL. Thanks for noticing. I'll fix before pushing. Anyone plans to review patch #3? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 1/3] Introduce gdb::function_view 2017-02-22 17:40 ` Pedro Alves 2017-02-22 17:49 ` [PATCH v1.1 " Pedro Alves @ 2017-02-22 18:02 ` Simon Marchi 1 sibling, 0 replies; 27+ messages in thread From: Simon Marchi @ 2017-02-22 18:02 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches On 2017-02-22 12:40, Pedro Alves wrote: > On 02/22/2017 03:15 PM, Simon Marchi wrote: >>> + >>> + This is meant to be used as callback type of a function that: >>> + >>> + #1 - Takes a callback as parameter. >>> + >>> + #2 - Does not store the callback anywhere; instead if just >>> calls >> >> if -> it >> >>> + it or forwards it to some other function that calls it. > > Thanks. Maybe that's too many confusing "it"s. I've rewritten it to > > #2 - Does not store the callback anywhere; instead the function > just calls the callback directly or forwards it to some > other function that calls it. LGTM. >>> + >>> + Usage: >>> + >>> + Given this function that accepts a callback: >> >> It's not necessary, but it would be nice to have the equivalent >> example >> of how it would've been done before (with a function pointer) so that >> people can relate the following example to something they already >> know. > > I wasn't sure whether that should be here, thinking it might > be more appropriate for the internals manual? I would paste > an example using a callable as template parameter there too, for > example. But I guess writing it too can't hurt, since that's likely > where people will look first when they want to know what are those > "function_view"s in the source. > > I've extended this section in that direction, and tried to clarify > other bits along with it. See below. > >> >>> + void >>> + iterate_over_foos (gdb::function_view<void (foo *)> callback) >>> + { >>> + for (auto & : foos) >> >> I think you're missing a "foo" here. > > Indeed. Found another buglet in the examples that I fixed too. > >>> +namespace traits >>> +{ >> >> Is it intended to have this { on a separate line, unlike the other >> namespace declarations? > > Nope, somehow missed it. > >> I am not going to try to understand any of this... but as long as it >> works I'm happy. > > Yeah, these library facilities tend to require use of C++ that is > not normally used by "regular" code. In isolation, none of the > features used is really complicated, but when they're all combined, > it gives the impression otherwise. The advantage is that it > ends up all contained in a single place, while users of the library > end up being simple and efficient. Let me know if I can explain > things better. > > Here's the tweak I mean to squash down addressing your review. > I also realized that the unit test had some formatting issues, and > the symbol names uses there could be renamed to make things a bit > clearer. > > I'll send the updated (squashed) patch as a reply. > > From 79ce1e9b5fc1e69747d033fc384c7f50c61be1ba Mon Sep 17 00:00:00 2001 > From: Pedro Alves <palves@redhat.com> > Date: Wed, 22 Feb 2017 17:30:00 +0000 > Subject: [PATCH] review > > --- > gdb/common/function-view.h | 104 > ++++++++++++++++++++---------- > gdb/unittests/function-view-selftests.c | 109 > ++++++++++++++++---------------- > 2 files changed, 125 insertions(+), 88 deletions(-) > > diff --git a/gdb/common/function-view.h b/gdb/common/function-view.h > index 3303783..af2593f 100644 > --- a/gdb/common/function-view.h > +++ b/gdb/common/function-view.h > @@ -30,33 +30,67 @@ > > #1 - Takes a callback as parameter. > > - #2 - Does not store the callback anywhere; instead if just calls > - it or forwards it to some other function that calls it. > - > - #3 - When we don't want, or can't make said function be a > - template function with the callable type as template > - parameter. For example, when the callback is a parameter of > - a virtual member function, or when putting the function > - template in a header would expose too much implementation > - detail. > - > - For this use case, which is quite pervasive, a function_view is a > - better choice for callback type than std::function. It is a better > - choice because std::function is a heavy-weight object with value > + #2 - Wants to support arbitrary callable objects as callback type > + (e.g., stateful function objects, lambda closures, free > + functions). > + > + #3 - Does not store the callback anywhere; instead the function > + just calls the callback directly or forwards it to some > + other function that calls it. > + > + #4 - Can't be, or we don't want it to be, a template function > + with the callable type as template parameter. For example, > + when the callback is a parameter of a virtual member > + function, or when putting the function template in a header > + would expose too much implementation detail. > + > + Note that the C-style "function pointer" + "void *data" callback > + parameter idiom fails requirement #2 above. Please don't add new > + uses of that idiom. I.e., something like this wouldn't work; > + > + typedef bool (iterate_over_foos_cb) (foo *f, void *user_data), > + void iterate_over_foos (iterate_over_foos_cb *callback, void > *user_data); > + > + foo *find_foo_by_type (int type) > + { > + foo *found = nullptr; > + > + iterate_over_foos ([&] (foo *f, void *data) > + { > + if (foo->type == type) > + { > + found = foo; > + return true; // stop iterating > + } > + return false; // continue iterating > + }, NULL); > + > + return found; > + } > + > + The above wouldn't compile, because lambdas with captures can't be > + implicitly converted to a function pointer (because a capture means > + some context data must be passed to the lambda somehow). > + > + C++11 gave us std::function as type-erased wrapper around arbitrary > + callables, however, std::function is not an ideal fit for transient > + callbacks such as the use case above. For this use case, which is > + quite pervasive, a function_view is a better choice, because while > + while function_view is light and does not require any heap > + allocation, std::function is a heavy-weight object with value > semantics that generally requires a heap allocation on > - construction/assignment of the target callable, while function_view > - is light and does not require any heap allocation. It _is_ > - possible to use std::function in such a way that avoids most of the > - overhead by making sure to only construct it with callables of > - types that fit std::function's small object optimization, such as > - function pointers and std::reference_wrapper callables, however, > - that is quite inconvenient in practice, because restricting to > - free-function callables would imply no state/capture (which we need > - in most cases), and std::reference_wrapper implies remembering to > - use std::ref/std::cref where the callable is constructed, with the > - added inconvenience that those function have deleted rvalue-ref > - overloads, meaning you can't use unnamed/temporary lambdas with > - them. > + construction/assignment of the target callable. In addition, while > + it is possible to use std::function in such a way that avoids most > + of the overhead by making sure to only construct it with callables > + of types that fit std::function's small object optimization, such > + as function pointers and std::reference_wrapper callables, that is > + quite inconvenient in practice, because restricting to > + free-function callables would imply no state/capture/closure, which > + we need in most cases, and std::reference_wrapper implies > + remembering to use std::ref/std::cref where the callable is > + constructed, with the added inconvenience that std::ref/std::cref > + have deleted rvalue-ref overloads, meaning you can't use > + unnamed/temporary lambdas with them. > > Note that because function_view is a non-owning view of a callable, > care must be taken to ensure that the callable outlives the > @@ -81,15 +115,16 @@ > void > iterate_over_foos (gdb::function_view<void (foo *)> callback) > { > - for (auto & : foos) > - callback (&foo); > + for (auto &foo : foos) > + callback (&foo); > } > > you can call it like this, passing a lambda as callback: > > - iterate_over_foos ([&] (foo *f) { > - process_one_foo (f); > - }); > + iterate_over_foos ([&] (foo *f) > + { > + process_one_foo (f); > + }); > > or like this, passing a function object as callback: > > @@ -97,15 +132,16 @@ > { > void operator() (foo *f) > { > - if (s->check ()) > - process_one_foo (f); > + if (s->check ()) > + process_one_foo (f); > } > > // some state > state *s; > }; > > - function_object matcher (mystate); > + state mystate; > + function_object matcher {&mystate}; > iterate_over_foos (matcher); > > or like this, passing a function pointer as callback: > diff --git a/gdb/unittests/function-view-selftests.c > b/gdb/unittests/function-view-selftests.c > index 8f73bc4..3e5369b 100644 > --- a/gdb/unittests/function-view-selftests.c > +++ b/gdb/unittests/function-view-selftests.c > @@ -27,143 +27,144 @@ namespace selftests { > namespace function_view { > > static int > -add_one (int count) > +plus_one_fn_int (int val) > { > - return ++count; > + return ++val; > } > > static short > -add_one_short (short count) > +plus_one_fn_short (short val) > { > - return ++count; > + return ++val; > } > > static int > -call_callback (int count, gdb::function_view <int(int)> callback) > +call_callback_int (int val, gdb::function_view <int (int)> callback) > { > - return callback (count); > + return callback (val); > } > > static void > -call_callback_void (int count, gdb::function_view <void(int)> > callback) > +call_callback_void (int val, gdb::function_view <void (int)> callback) > { > - callback (count); > + callback (val); > } > > -struct plus_one_function_object > +struct plus_one_int_func_obj > { > - int operator () (int count) > + int operator () (int val) > { > - ++calls; > - return ++count; > + ++call_count; > + return ++val; > } > > - int calls = 0; > + /* Number of times called. */ > + int call_count = 0; > }; > > static void > run_tests () > { > /* A simple lambda. */ > - auto plus_one = [] (int count) { return ++count; }; > + auto plus_one_lambda = [] (int val) { return ++val; }; > > /* A function_view that references the lambda. */ > - gdb::function_view<int (int)> plus_one_view (plus_one); > + gdb::function_view<int (int)> plus_one_func_view (plus_one_lambda); > > /* Check calling the lambda directly. */ > - SELF_CHECK (plus_one (0) == 1); > - SELF_CHECK (plus_one (1) == 2); > + SELF_CHECK (plus_one_lambda (0) == 1); > + SELF_CHECK (plus_one_lambda (1) == 2); > > /* Check calling lambda via the view. */ > - SELF_CHECK (plus_one_view (2) == 3); > - SELF_CHECK (plus_one_view (3) == 4); > + SELF_CHECK (plus_one_func_view (2) == 3); > + SELF_CHECK (plus_one_func_view (3) == 4); > > /* Check calling a function that takes a function_view as argument, > by value. Pass a lambda, making sure a function_view is properly > constructed implicitly. */ > - SELF_CHECK (call_callback (1, [] (int count) > + SELF_CHECK (call_callback_int (1, [] (int val) > { > - return count + 2; > + return val + 2; > }) == 3); > > /* Same, passing a named/lvalue lambda. */ > - SELF_CHECK (call_callback (1, plus_one) == 2); > + SELF_CHECK (call_callback_int (1, plus_one_lambda) == 2); > /* Same, passing named/lvalue function_view (should copy). */ > - SELF_CHECK (call_callback (1, plus_one_view) == 2); > + SELF_CHECK (call_callback_int (1, plus_one_func_view) == 2); > > /* Check constructing a function view over a function-object > callable, and calling it. */ > - plus_one_function_object func_obj; > + plus_one_int_func_obj func_obj; > SELF_CHECK (func_obj (0) == 1); > - SELF_CHECK (call_callback (1, func_obj) == 2); > + SELF_CHECK (call_callback_int (1, func_obj) == 2); > /* Check that the callable was referenced, not copied. */ > - SELF_CHECK (func_obj.calls == 2); > + SELF_CHECK (func_obj.call_count == 2); > > - /* Check constructing a function_view over free-function callable, > + /* Check constructing a function_view over a free-function callable, > and calling it. */ > - SELF_CHECK (call_callback (1, add_one) == 2); > + SELF_CHECK (call_callback_int (1, plus_one_fn_int) == 2); > > /* Check calling a function with a > compatible-but-not-exactly-the-same prototype. */ > - SELF_CHECK (call_callback (1, [] (short count) -> short > + SELF_CHECK (call_callback_int (1, [] (short val) -> short > { > - return count + 2; > + return val + 2; > }) == 3); > /* Same, but passing a function pointer. */ > - SELF_CHECK (call_callback (1, add_one_short) == 2); > + SELF_CHECK (call_callback_int (1, plus_one_fn_short) == 2); > > /* Like std::function, a function_view that expects a void return > can reference callables with non-void return type. The result is > simply discarded. Check a lambda, function object and a function > pointer. */ > - call_callback_void (1, [] (int count) -> int > + call_callback_void (1, [] (int val) -> int > { > - return count + 2; > + return val + 2; > }); > call_callback_void (1, func_obj); > - call_callback_void (1, add_one); > + call_callback_void (1, plus_one_fn_int); > > /* Check that the main ctor doesn't hijack the copy ctor. */ > - auto plus_one_view2 (plus_one_view); > - auto plus_one_view3 (plus_one_view2); > - static_assert (std::is_same<decltype (plus_one_view), > - decltype (plus_one_view2)>::value, ""); > - static_assert (std::is_same<decltype (plus_one_view), > - decltype (plus_one_view3)>::value, ""); > + auto plus_one_func_view2 (plus_one_func_view); > + auto plus_one_func_view3 (plus_one_func_view2); > + static_assert (std::is_same<decltype (plus_one_func_view), > + decltype (plus_one_func_view2)>::value, ""); > + static_assert (std::is_same<decltype (plus_one_func_view), > + decltype (plus_one_func_view3)>::value, ""); > > - SELF_CHECK (plus_one_view3 (1) == 2); > + SELF_CHECK (plus_one_func_view3 (1) == 2); > > /* Likewise, but propagate a NULL callable. If this calls the main > function_view ctor instead of the copy ctor by mistake, then > null_func_2 ends up non-NULL (because it'd instead reference > null_func_1 as just another callable). */ > - constexpr gdb::function_view<int(int)> null_func_1 = nullptr; > - constexpr auto null_func_2 (null_func_1); > + constexpr gdb::function_view<int (int)> null_func_view_1 = nullptr; > + constexpr auto null_func_view_2 (null_func_view_1); > > /* While at it, check whether the function_view is bound using > various forms, op==, op!= and op bool. */ > > /* op== */ > - static_assert (null_func_2 == nullptr, ""); > - static_assert (nullptr == null_func_2, ""); > - static_assert (null_func_2 == NULL, ""); > - static_assert (NULL == null_func_2, ""); > + static_assert (null_func_view_2 == nullptr, ""); > + static_assert (nullptr == null_func_view_2, ""); > + static_assert (null_func_view_2 == NULL, ""); > + static_assert (NULL == null_func_view_2, ""); > > /* op!= */ > - static_assert (!(null_func_2 != nullptr), ""); > - static_assert (!(nullptr != null_func_2), ""); > - static_assert (!(null_func_2 != NULL), ""); > - static_assert (!(NULL != null_func_2), ""); > + static_assert (!(null_func_view_2 != nullptr), ""); > + static_assert (!(nullptr != null_func_view_2), ""); > + static_assert (!(null_func_view_2 != NULL), ""); > + static_assert (!(NULL != null_func_view_2), ""); > > /* op bool */ > - static_assert (!null_func_2, ""); > + static_assert (!null_func_view_2, ""); > > /* Check the nullptr_t ctor. */ > - constexpr gdb::function_view<int(int)> check_ctor_nullptr (nullptr); > + constexpr gdb::function_view<int (int)> check_ctor_nullptr > (nullptr); > static_assert (!check_ctor_nullptr, ""); > > /* Check the nullptr_t op= */ > - gdb::function_view<int(int)> check_op_eq_null (add_one); > + gdb::function_view<int (int)> check_op_eq_null (plus_one_fn_int); > SELF_CHECK (check_op_eq_null); > check_op_eq_null = nullptr; > SELF_CHECK (!check_op_eq_null); That's fine with me. ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2017-02-24 17:45 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-02-22 14:50 [PATCH 0/3] Introduce gdb::function_view & fix completion bug Pedro Alves 2017-02-22 14:50 ` [PATCH 2/3] Use gdb::function_view in iterate_over_symtabs & co Pedro Alves 2017-02-22 22:40 ` Yao Qi 2017-02-22 14:50 ` [PATCH 3/3] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves 2017-02-23 16:02 ` Yao Qi 2017-02-23 17:12 ` Pedro Alves 2017-02-23 17:24 ` [PATCH v2 0/2] " Pedro Alves 2017-02-23 17:24 ` [PATCH v2 1/2] symtab.c: Small refactor Pedro Alves 2017-02-23 21:45 ` Yao Qi 2017-02-24 17:45 ` Pedro Alves 2017-02-23 17:24 ` [PATCH v2 2/2] Fix gdb.base/completion.exp with --target_board=dwarf4-gdb-index Pedro Alves 2017-02-24 15:34 ` Yao Qi 2017-02-24 17:15 ` Pedro Alves 2017-02-22 14:50 ` [PATCH 1/3] Introduce gdb::function_view Pedro Alves 2017-02-22 15:15 ` Simon Marchi 2017-02-22 17:40 ` Pedro Alves 2017-02-22 17:49 ` [PATCH v1.1 " Pedro Alves 2017-02-22 22:12 ` Yao Qi 2017-02-23 14:49 ` Pedro Alves 2017-02-23 15:11 ` Yao Qi 2017-02-23 15:20 ` Pedro Alves 2017-02-23 15:34 ` Yao Qi 2017-02-22 22:23 ` Yao Qi 2017-02-23 14:50 ` [PATCH v1.2 " Pedro Alves 2017-02-23 14:58 ` Yao Qi 2017-02-23 14:59 ` Pedro Alves 2017-02-22 18:02 ` [PATCH " Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox