Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On May 28, 2018 4:25:02 PM GMT+02:00, Bernd Edlinger wrote: >On 05/28/18 11:19, Richard Biener wrote: >> On Sat, May 26, 2018 at 10:19 AM Bernd Edlinger > >> wrote: >> >> >> >>> On 05/17/18 16:37, Bernd Edlinger wrote: On 05/17/18 15:39, Richard Biener wrote: > On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger > > wrote: > >> Ping... > > So this makes all traditional users go through the indirect > splay_tree_compare_wrapper > and friends (which is also exported for no good reason?). And all >> users > are traditional > at the moment. > all except gcc/typed-splay-tree.h which only works if VALUE_TYPE is compatible with uint_ptr_t but cannot check this requirement. This one worried me the most. But not having to rewrite omp-low.c for instance where >splay_tree_lookup and access to n->value are made all the time, made me think it will not work to rip out the old interface completely. >> >>> Well, I think it will be best to split this patch in two parts: >> >>> One that adds just two utility functions for avoiding undefined >>> function type casts which can be used with the original C interface. >>> This first part is attached. >> >>> And another part that uses a similar approach as the splay-tree in >>> libgomp, but instead of creating a type-safe C interface it should >>> translate the complete code from splay-tree.c/.h into a template. >>> The second part, I plan to do at a later time. >> >> >>> Is this OK for trunk? >> >> Looks ok to me. Do we need to worry about !HAVE_STRING_H and >> using strcmp? >> > >No, I would be rather surprised if libiberty would compile at all >without string.h. First some files include it without HAVE_STRING_H >for instance sha1.c and argv.c, so I just replicated what >the majority of the code base here did. > >Most sources include ifdef HAVE_STRING_H, and use strcmp >even if it is not declared (which should work for functions with int >result). > >So I would commit this patch as is, if you agree. Sure - go ahead. Richard. > >Bernd. > > >> Thanks, >> Richard. >> >> >>> Thanks >>> Bernd.
Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On 05/28/18 11:19, Richard Biener wrote: > On Sat, May 26, 2018 at 10:19 AM Bernd Edlinger > wrote: > > > >> On 05/17/18 16:37, Bernd Edlinger wrote: >>> On 05/17/18 15:39, Richard Biener wrote: On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger wrote: > Ping... So this makes all traditional users go through the indirect splay_tree_compare_wrapper and friends (which is also exported for no good reason?). And all > users are traditional at the moment. >>> >>> all except gcc/typed-splay-tree.h which only works if VALUE_TYPE is >>> compatible with uint_ptr_t but cannot check this requirement. >>> This one worried me the most. >>> >>> But not having to rewrite omp-low.c for instance where splay_tree_lookup >>> and access to n->value are made all the time, made me think it >>> will not work to rip out the old interface completely. >>> > >> Well, I think it will be best to split this patch in two parts: > >> One that adds just two utility functions for avoiding undefined >> function type casts which can be used with the original C interface. >> This first part is attached. > >> And another part that uses a similar approach as the splay-tree in >> libgomp, but instead of creating a type-safe C interface it should >> translate the complete code from splay-tree.c/.h into a template. >> The second part, I plan to do at a later time. > > >> Is this OK for trunk? > > Looks ok to me. Do we need to worry about !HAVE_STRING_H and > using strcmp? > No, I would be rather surprised if libiberty would compile at all without string.h. First some files include it without HAVE_STRING_H for instance sha1.c and argv.c, so I just replicated what the majority of the code base here did. Most sources include ifdef HAVE_STRING_H, and use strcmp even if it is not declared (which should work for functions with int result). So I would commit this patch as is, if you agree. Bernd. > Thanks, > Richard. > > >> Thanks >> Bernd.
Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On Sat, May 26, 2018 at 10:19 AM Bernd Edlinger wrote: > On 05/17/18 16:37, Bernd Edlinger wrote: > > On 05/17/18 15:39, Richard Biener wrote: > >> On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger > >> > >> wrote: > >> > >>> Ping... > >> > >> So this makes all traditional users go through the indirect > >> splay_tree_compare_wrapper > >> and friends (which is also exported for no good reason?). And all users > >> are traditional > >> at the moment. > >> > > > > all except gcc/typed-splay-tree.h which only works if VALUE_TYPE is > > compatible with uint_ptr_t but cannot check this requirement. > > This one worried me the most. > > > > But not having to rewrite omp-low.c for instance where splay_tree_lookup > > and access to n->value are made all the time, made me think it > > will not work to rip out the old interface completely. > > > Well, I think it will be best to split this patch in two parts: > One that adds just two utility functions for avoiding undefined > function type casts which can be used with the original C interface. > This first part is attached. > And another part that uses a similar approach as the splay-tree in > libgomp, but instead of creating a type-safe C interface it should > translate the complete code from splay-tree.c/.h into a template. > The second part, I plan to do at a later time. > Is this OK for trunk? Looks ok to me. Do we need to worry about !HAVE_STRING_H and using strcmp? Thanks, Richard. > Thanks > Bernd.
Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On 05/17/18 16:37, Bernd Edlinger wrote: > On 05/17/18 15:39, Richard Biener wrote: >> On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger >> >> wrote: >> >>> Ping... >> >> So this makes all traditional users go through the indirect >> splay_tree_compare_wrapper >> and friends (which is also exported for no good reason?). And all users >> are traditional >> at the moment. >> > > all except gcc/typed-splay-tree.h which only works if VALUE_TYPE is > compatible with uint_ptr_t but cannot check this requirement. > This one worried me the most. > > But not having to rewrite omp-low.c for instance where splay_tree_lookup > and access to n->value are made all the time, made me think it > will not work to rip out the old interface completely. > Well, I think it will be best to split this patch in two parts: One that adds just two utility functions for avoiding undefined function type casts which can be used with the original C interface. This first part is attached. And another part that uses a similar approach as the splay-tree in libgomp, but instead of creating a type-safe C interface it should translate the complete code from splay-tree.c/.h into a template. The second part, I plan to do at a later time. Is this OK for trunk? Thanks Bernd. include: 2018-05-26 Bernd Edlinger * splay-tree.h (splay_tree_compare_strings, splay_tree_delete_pointers): Declare new utility functions. libiberty: 2018-05-26 Bernd Edlinger * splay-tree.c (splay_tree_compare_strings, splay_tree_delete_pointers): New utility functions. gcc: 2018-05-26 Bernd Edlinger * tree-dump.c (dump_node): Use splay_tree_delete_pointers. c-family: 2018-05-26 Bernd Edlinger * c-lex.c (get_fileinfo): Use splay_tree_compare_strings and splay_tree_delete_pointers. cp: 2018-05-26 Bernd Edlinger * decl2.c (start_static_storage_duration_function): Use splay_tree_delete_pointers. Index: gcc/c-family/c-lex.c === --- gcc/c-family/c-lex.c (revision 260671) +++ gcc/c-family/c-lex.c (working copy) @@ -103,11 +103,9 @@ get_fileinfo (const char *name) struct c_fileinfo *fi; if (!file_info_tree) -file_info_tree = splay_tree_new ((splay_tree_compare_fn) - (void (*) (void)) strcmp, +file_info_tree = splay_tree_new (splay_tree_compare_strings, 0, - (splay_tree_delete_value_fn) - (void (*) (void)) free); + splay_tree_delete_pointers); n = splay_tree_lookup (file_info_tree, (splay_tree_key) name); if (n) Index: gcc/cp/decl2.c === --- gcc/cp/decl2.c (revision 260671) +++ gcc/cp/decl2.c (working copy) @@ -3595,8 +3595,7 @@ start_static_storage_duration_function (unsigned c priority_info_map = splay_tree_new (splay_tree_compare_ints, /*delete_key_fn=*/0, /*delete_value_fn=*/ - (splay_tree_delete_value_fn) - (void (*) (void)) free); + splay_tree_delete_pointers); /* We always need to generate functions for the DEFAULT_INIT_PRIORITY so enter it now. That way when we walk Index: gcc/tree-dump.c === --- gcc/tree-dump.c (revision 260671) +++ gcc/tree-dump.c (working copy) @@ -736,8 +736,7 @@ dump_node (const_tree t, dump_flags_t flags, FILE di.flags = flags; di.node = t; di.nodes = splay_tree_new (splay_tree_compare_pointers, 0, - (splay_tree_delete_value_fn) - (void (*) (void)) free); + splay_tree_delete_pointers); /* Queue up the first node. */ queue (&di, t, DUMP_NONE); Index: include/splay-tree.h === --- include/splay-tree.h (revision 260671) +++ include/splay-tree.h (working copy) @@ -147,7 +147,9 @@ extern splay_tree_node splay_tree_max (splay_tree) extern splay_tree_node splay_tree_min (splay_tree); extern int splay_tree_foreach (splay_tree, splay_tree_foreach_fn, void*); extern int splay_tree_compare_ints (splay_tree_key, splay_tree_key); -extern int splay_tree_compare_pointers (splay_tree_key, splay_tree_key); +extern int splay_tree_compare_pointers (splay_tree_key, splay_tree_key); +extern int splay_tree_compare_strings (splay_tree_key, splay_tree_key); +extern void splay_tree_delete_pointers (splay_tree_value); #ifdef __cplusplus } Index: libiberty/splay-tree.c === --- libiberty/splay-tree.c (revision 260671) +++ libiberty/splay-tree.c (working copy) @@ -31,6 +31,9 @@ Boston, MA 02110-1301, USA. */ #ifdef HAVE_STDLIB_H #include #endif +#ifdef HAVE_STRING_H +#include +#endif #include @@ -590,3 +593,19 @@ splay_tree_compare_pointers (splay_tree_key k1, sp else return 0; } + +/* Splay-tree comparison function, treating the keys as strings. */ + +int +splay_tree_compare_s
Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On 05/17/18 15:39, Richard Biener wrote: > On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger > wrote: > >> Ping... > > So this makes all traditional users go through the indirect > splay_tree_compare_wrapper > and friends (which is also exported for no good reason?). And all users > are traditional > at the moment. > all except gcc/typed-splay-tree.h which only works if VALUE_TYPE is compatible with uint_ptr_t but cannot check this requirement. This one worried me the most. But not having to rewrite omp-low.c for instance where splay_tree_lookup and access to n->value are made all the time, made me think it will not work to rip out the old interface completely. Bernd. > So I wonder if it's better to have a complete alternate interface? I do > not see many > users besides gcc, there's a use in bfd elf32-xtensa.c and some uses in > gdb. Of course > disregarding any users outside of SRC. > > Richard. > > >> On 05/03/18 22:13, Bernd Edlinger wrote: >>> Hi, >>> >>> this is basically the same patch I posted a few months ago, >>> with a few formatting nits by Jakub fixed. >>> >>> Bootstrapped and reg-tested again with current trunk. >>> >>> Is it OK for trunk? >>> >>> >>> Bernd. >>> >>> On 12/15/17 11:44, Bernd Edlinger wrote: Hi, when working on the -Wcast-function-type patch I noticed some rather ugly and non-portable function type casts that are necessary to accomplish some actually very simple tasks. Often functions taking pointer arguments are called with a different signature taking uintptr_t arguments, which is IMHO not really safe to do... The attached patch adds a context argument to the callback functions > but keeps the existing interface as far as possible. Bootstrapped and reg-tested on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd.
Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On Thu, May 17, 2018 at 4:04 PM Jakub Jelinek wrote: > On Thu, May 17, 2018 at 03:39:42PM +0200, Richard Biener wrote: > > On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger < bernd.edlin...@hotmail.de> > > wrote: > > > > > Ping... > > > > So this makes all traditional users go through the indirect > > splay_tree_compare_wrapper > > and friends (which is also exported for no good reason?). And all users > > are traditional > > at the moment. > > > > So I wonder if it's better to have a complete alternate interface? I do > > not see many > > users besides gcc, there's a use in bfd elf32-xtensa.c and some uses in > libgomp has a copy (intentionally so, it is slightly changed). So if that is not exported ABI wise then maybe we can unshare it again into an even more improved splay_tree2 in libiberty and deprecate the "old" one? DJ, how's "compatibility" supposed to work with libiberty? Richard. > Jakub
Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On Thu, May 17, 2018 at 03:39:42PM +0200, Richard Biener wrote: > On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger > wrote: > > > Ping... > > So this makes all traditional users go through the indirect > splay_tree_compare_wrapper > and friends (which is also exported for no good reason?). And all users > are traditional > at the moment. > > So I wonder if it's better to have a complete alternate interface? I do > not see many > users besides gcc, there's a use in bfd elf32-xtensa.c and some uses in libgomp has a copy (intentionally so, it is slightly changed). Jakub
Re: [PING] [PATCH] Avoid excessive function type casts with splay-trees
On Thu, May 17, 2018 at 3:21 PM Bernd Edlinger wrote: > Ping... So this makes all traditional users go through the indirect splay_tree_compare_wrapper and friends (which is also exported for no good reason?). And all users are traditional at the moment. So I wonder if it's better to have a complete alternate interface? I do not see many users besides gcc, there's a use in bfd elf32-xtensa.c and some uses in gdb. Of course disregarding any users outside of SRC. Richard. > On 05/03/18 22:13, Bernd Edlinger wrote: > > Hi, > > > > this is basically the same patch I posted a few months ago, > > with a few formatting nits by Jakub fixed. > > > > Bootstrapped and reg-tested again with current trunk. > > > > Is it OK for trunk? > > > > > > Bernd. > > > > On 12/15/17 11:44, Bernd Edlinger wrote: > >> Hi, > >> > >> when working on the -Wcast-function-type patch I noticed some rather > >> ugly and non-portable function type casts that are necessary to > >> accomplish > >> some actually very simple tasks. > >> > >> Often functions taking pointer arguments are called with a different > >> signature > >> taking uintptr_t arguments, which is IMHO not really safe to do... > >> > >> The attached patch adds a context argument to the callback functions but > >> keeps the existing interface as far as possible. > >> > >> > >> Bootstrapped and reg-tested on x86_64-pc-linux-gnu. > >> Is it OK for trunk? > >> > >> > >> Thanks > >> Bernd. > >>
