| Message ID | 8423b154-14ac-0263-aa18-01752c6894ef@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] hash-table for extern-c fns. | expand |
On Fri, Oct 6, 2017 at 10:18 AM, Nathan Sidwell <nathan@acm.org> wrote: > This patch converts the extern "C" function map to use a hash table, in the > same way as I've just changed namespace bindings. > > There's a small wart, in that the c_linkage_bindings user (in c-common) > expects either a single decl or a TREE_LIST. I.e. not an OVERLOAD. But the > hasher expects either a DECL or an OVERLOAD. Rather than extend the hasher > (and marginally pessimize it for this special case), I modify the extern-c > table handling to insert an initial OVERLOAD node, when there are multiple > functions. That can CHAIN directly to a TREE_LIST. Sure, we have a wasted > OVERLOAD node in this case, but it's going to be rare -- programs don't > usually declare extern "C" functions of the same name in different > namespaces. > > Changing the c-common function is harder, as OVERLOAD is a C++ FE local > node. Hmm, why do we only check extern "C" conflicts for functions? Jason
On 10/09/2017 11:57 AM, Jason Merrill wrote: > On Fri, Oct 6, 2017 at 10:18 AM, Nathan Sidwell <nathan@acm.org> wrote: >> This patch converts the extern "C" function map to use a hash table, in the >> same way as I've just changed namespace bindings. > Hmm, why do we only check extern "C" conflicts for functions? I suspect a bug. I noticed it as existing behaviour and was puzzled the first time around rearranging this code, but didn't want to get distracted from the big picture. nathan
On 10/09/2017 06:15 PM, Nathan Sidwell wrote: > On 10/09/2017 11:57 AM, Jason Merrill wrote: >> Hmm, why do we only check extern "C" conflicts for functions? > > I suspect a bug. I noticed it as existing behaviour and was puzzled the > first time around rearranging this code, but didn't want to get > distracted from the big picture. This patch checks both vars and fns (as typedefs don't have linkage, I don't think they can be extern C, but ICBW). I also tweaked the diagnostics, so that the first one is a pedwarn and the later ones are notes. (so we won't arbitrarily truncate the series of diagnostics. Look good to you? nathan
On Tue, Oct 10, 2017 at 3:11 PM, Nathan Sidwell <nathan@acm.org> wrote: > On 10/09/2017 06:15 PM, Nathan Sidwell wrote: >> >> On 10/09/2017 11:57 AM, Jason Merrill wrote: > > >>> Hmm, why do we only check extern "C" conflicts for functions? >> >> >> I suspect a bug. I noticed it as existing behaviour and was puzzled the >> first time around rearranging this code, but didn't want to get distracted >> from the big picture. > > > This patch checks both vars and fns (as typedefs don't have linkage, I don't > think they can be extern C, but ICBW). > > I also tweaked the diagnostics, so that the first one is a pedwarn and the > later ones are notes. (so we won't arbitrarily truncate the series of > diagnostics. > > Look good to you? Yes, thanks.
2017-10-06 Nathan Sidwell <nathan@acm.org> Use hash_table for extern "C" names * name-lookup.c (extern_c_fns): Use hash_table. (check_extern_c_conflict): Adjust. (c_linkage_bindings): Adjust. Index: name-lookup.c =================================================================== --- name-lookup.c (revision 253489) +++ name-lookup.c (working copy) @@ -2511,9 +2511,9 @@ update_binding (cp_binding_level *level, return decl; } -/* Map of identifiers to extern C functions (or LISTS thereof). */ +/* Table of identifiers to extern C functions (or LISTS thereof). */ -static GTY(()) hash_map<lang_identifier *, tree> *extern_c_fns; +static GTY(()) hash_table<named_decl_hash> *extern_c_fns; /* DECL has C linkage. If we have an existing instance, make sure it has the same exception specification [7.5, 7.6]. If there's no @@ -2527,17 +2527,15 @@ check_extern_c_conflict (tree decl) return; if (!extern_c_fns) - extern_c_fns = hash_map<lang_identifier *,tree>::create_ggc (127); + extern_c_fns = hash_table<named_decl_hash>::create_ggc (127); - bool existed; - tree *slot = &extern_c_fns->get_or_insert (DECL_NAME (decl), &existed); - if (!existed) - *slot = decl; - else + tree *slot = extern_c_fns + ->find_slot_with_hash (DECL_NAME (decl), + IDENTIFIER_HASH_VALUE (DECL_NAME (decl)), INSERT); + if (tree old = *slot) { - tree old = *slot; - if (TREE_CODE (old) == TREE_LIST) - old = TREE_VALUE (old); + if (TREE_CODE (old) == OVERLOAD) + old = OVL_FUNCTION (old); int mismatch = 0; if (DECL_CONTEXT (old) == DECL_CONTEXT (decl)) @@ -2563,9 +2561,24 @@ check_extern_c_conflict (tree decl) "due to different exception specifications"); } else - /* Chain it on for c_linkage_binding's use. */ - *slot = tree_cons (NULL_TREE, decl, *slot); + { + if (old == *slot) + /* The hash table expects OVERLOADS, so construct one with + OLD as both the function and the chain. This allocate + an excess OVERLOAD node, but it's rare to have multiple + extern "C" decls of the same name. And we save + complicating the hash table logic (which is used + elsewhere). */ + *slot = ovl_make (old, old); + + slot = &OVL_CHAIN (*slot); + + /* Chain it on for c_linkage_binding's use. */ + *slot = tree_cons (NULL_TREE, decl, *slot); + } } + else + *slot = decl; } /* Returns a list of C-linkage decls with the name NAME. Used in @@ -2575,8 +2588,15 @@ tree c_linkage_bindings (tree name) { if (extern_c_fns) - if (tree *slot = extern_c_fns->get (name)) - return *slot; + if (tree *slot = extern_c_fns + ->find_slot_with_hash (name, IDENTIFIER_HASH_VALUE (name), NO_INSERT)) + { + tree result = *slot; + if (TREE_CODE (result) == OVERLOAD) + result = OVL_CHAIN (result); + return result; + } + return NULL_TREE; }