| Message ID | 522E6EFE.2080602@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 09/09/2013 06:59 PM, Andrew MacLeod wrote: > Whilst poking around, I discovered these hast table functions and the > macro are no longer used anywhere in the compiler. > bootstraps on x86_64-unknown-linux-gnu. > Probably obvious but in case I missed something... OK? If they're unused, eliminate them. OK for the trunk. jeff
On Mon, Sep 09, 2013 at 08:39:16PM -0600, Jeff Law wrote: > On 09/09/2013 06:59 PM, Andrew MacLeod wrote: > >Whilst poking around, I discovered these hast table functions and the > >macro are no longer used anywhere in the compiler. > >bootstraps on x86_64-unknown-linux-gnu. > >Probably obvious but in case I missed something... OK? > If they're unused, eliminate them. OK for the trunk. Those macros were added last year together with hash-table.h, just nothing uses them so far, but they are counterparts of the hashtab.h traversal function which is used in various parts. So removing it is IMHO a mistake. Jakub
On 09/10/2013 01:27 AM, Jakub Jelinek wrote: > On Mon, Sep 09, 2013 at 08:39:16PM -0600, Jeff Law wrote: >> On 09/09/2013 06:59 PM, Andrew MacLeod wrote: >>> Whilst poking around, I discovered these hast table functions and the >>> macro are no longer used anywhere in the compiler. >>> bootstraps on x86_64-unknown-linux-gnu. >>> Probably obvious but in case I missed something... OK? >> If they're unused, eliminate them. OK for the trunk. > Those macros were added last year together with hash-table.h, > just nothing uses them so far, but they are counterparts of the > hashtab.h traversal function which is used in various parts. > > So removing it is IMHO a mistake. > > Are you sure? FOR_EACH_HASH_TABLE_ELEMENT was added in April of this year as part of the hash table work, and it is used in a number of places. This one (FOR_EACH_HTAB_ELEMENT) was added in 2005 and would seem to be obsolete now... Andrew Andrew
On Tue, Sep 10, 2013 at 07:42:42AM -0400, Andrew MacLeod wrote: > On 09/10/2013 01:27 AM, Jakub Jelinek wrote: > >On Mon, Sep 09, 2013 at 08:39:16PM -0600, Jeff Law wrote: > >>On 09/09/2013 06:59 PM, Andrew MacLeod wrote: > >>>Whilst poking around, I discovered these hast table functions and the > >>>macro are no longer used anywhere in the compiler. > >>>bootstraps on x86_64-unknown-linux-gnu. > >>>Probably obvious but in case I missed something... OK? > >>If they're unused, eliminate them. OK for the trunk. > >Those macros were added last year together with hash-table.h, > >just nothing uses them so far, but they are counterparts of the > >hashtab.h traversal function which is used in various parts. > > > >So removing it is IMHO a mistake. > > > > > Are you sure? FOR_EACH_HASH_TABLE_ELEMENT was added in April of > this year as part of the hash table work, and it is used in a number > of places. This one (FOR_EACH_HTAB_ELEMENT) was added in 2005 and > would seem to be obsolete now... I stand corrected, still I think it is a mistake to remove it, because the C++ alternative to htab_t only supports a subset of uses of the older C htab_t, e.g. it can't cope with GC. So FTTB some of the hash tables in the newly added code (most recently e.g. ubsan) need to use the C htab_t and could make use of the traversal iterator infrastructure. Jakub
On 09/10/2013 08:30 AM, Jakub Jelinek wrote: > On Tue, Sep 10, 2013 at 07:42:42AM -0400, Andrew MacLeod wrote: >> On 09/10/2013 01:27 AM, Jakub Jelinek wrote: >>> On Mon, Sep 09, 2013 at 08:39:16PM -0600, Jeff Law wrote: >>>> On 09/09/2013 06:59 PM, Andrew MacLeod wrote: >>>>> Whilst poking around, I discovered these hast table functions and the >>>>> macro are no longer used anywhere in the compiler. >>>>> bootstraps on x86_64-unknown-linux-gnu. >>>>> Probably obvious but in case I missed something... OK? >>>> If they're unused, eliminate them. OK for the trunk. >>> Those macros were added last year together with hash-table.h, >>> just nothing uses them so far, but they are counterparts of the >>> hashtab.h traversal function which is used in various parts. >>> >>> So removing it is IMHO a mistake. >>> >>> >> Are you sure? FOR_EACH_HASH_TABLE_ELEMENT was added in April of >> this year as part of the hash table work, and it is used in a number >> of places. This one (FOR_EACH_HTAB_ELEMENT) was added in 2005 and >> would seem to be obsolete now... > I stand corrected, still I think it is a mistake to remove it, because > the C++ alternative to htab_t only supports a subset of uses of the older > C htab_t, e.g. it can't cope with GC. So FTTB some of the hash tables > in the newly added code (most recently e.g. ubsan) need to use the C > htab_t and could make use of the traversal iterator infrastructure. > > Really? If it were useful I would have thought they'd be using it now. In any case, it doesn't belong in tree-flow.h.. . shouldn't these functions and macros be in hashtab.h then? I could move them there... Andrew Andrew
On Tue, Sep 10, 2013 at 09:04:41AM -0400, Andrew MacLeod wrote: > Really? If it were useful I would have thought they'd be using it now. > > In any case, it doesn't belong in tree-flow.h.. . shouldn't these > functions and macros be in hashtab.h then? It doesn't belong to hashtab.h, because that is a libiberty API, this style of iterators is GCC specific. Jakub
On 09/10/2013 09:15 AM, Jakub Jelinek wrote: > On Tue, Sep 10, 2013 at 09:04:41AM -0400, Andrew MacLeod wrote: >> Really? If it were useful I would have thought they'd be using it now. >> >> In any case, it doesn't belong in tree-flow.h.. . shouldn't these >> functions and macros be in hashtab.h then? > It doesn't belong to hashtab.h, because that is a libiberty API, this > style of iterators is GCC specific. > > Doesn't seem to be anywhere obvious to put gcc specific hash table iterators at first glance. You seem to think we should keep them so unless you have a better idea, I'll put it into a new file gcc-hash.h which can be included when someone needs it... (or simply removed when determined to be obsolete.) Andrew
>>>>> "Jakub" == Jakub Jelinek <jakub@redhat.com> writes: Jakub> On Tue, Sep 10, 2013 at 09:04:41AM -0400, Andrew MacLeod wrote: >> Really? If it were useful I would have thought they'd be using it now. >> >> In any case, it doesn't belong in tree-flow.h.. . shouldn't these >> functions and macros be in hashtab.h then? Jakub> It doesn't belong to hashtab.h, because that is a libiberty API, this Jakub> style of iterators is GCC specific. FWIW I've occasionally wished that these iterators were in libiberty. Currently the only iteration interface for hashtab is callback-based, which is a pain. Also it seems to me that the iterators belong in hashtab so as not to violate the module boundary. Tom
* hash-table.h: Remove comment relating to deleted macro. * tree-flow-inline.h (first_htab_element, end_htab_p, next_htab_element): Delete unused functions. * tree-flow.h (FOR_EACH_HTAB_ELEMENT): Delete unused macro. Index: hash-table.h =================================================================== *** hash-table.h (revision 202414) --- hash-table.h (working copy) *************** hash_table <Descriptor, Allocator>::end *** 1050,1059 **** /* Iterate through the elements of hash_table HTAB, using hash_table <....>::iterator ITER, ! storing each element in RESULT, which is of type TYPE. ! ! This macro has this form for compatibility with the ! FOR_EACH_HTAB_ELEMENT currently defined in tree-flow.h. */ #define FOR_EACH_HASH_TABLE_ELEMENT(HTAB, RESULT, TYPE, ITER) \ for ((ITER) = (HTAB).begin (); \ --- 1050,1056 ---- /* Iterate through the elements of hash_table HTAB, using hash_table <....>::iterator ITER, ! storing each element in RESULT, which is of type TYPE. */ #define FOR_EACH_HASH_TABLE_ELEMENT(HTAB, RESULT, TYPE, ITER) \ for ((ITER) = (HTAB).begin (); \ Index: tree-flow-inline.h =================================================================== *** tree-flow-inline.h (revision 202414) --- tree-flow-inline.h (working copy) *************** gimple_vop (const struct function *fun) *** 42,92 **** return fun->gimple_df->vop; } - /* Initialize the hashtable iterator HTI to point to hashtable TABLE */ - - static inline void * - first_htab_element (htab_iterator *hti, htab_t table) - { - hti->htab = table; - hti->slot = table->entries; - hti->limit = hti->slot + htab_size (table); - do - { - PTR x = *(hti->slot); - if (x != HTAB_EMPTY_ENTRY && x != HTAB_DELETED_ENTRY) - break; - } while (++(hti->slot) < hti->limit); - - if (hti->slot < hti->limit) - return *(hti->slot); - return NULL; - } - - /* Return current non-empty/deleted slot of the hashtable pointed to by HTI, - or NULL if we have reached the end. */ - - static inline bool - end_htab_p (const htab_iterator *hti) - { - if (hti->slot >= hti->limit) - return true; - return false; - } - - /* Advance the hashtable iterator pointed to by HTI to the next element of the - hashtable. */ - - static inline void * - next_htab_element (htab_iterator *hti) - { - while (++(hti->slot) < hti->limit) - { - PTR x = *(hti->slot); - if (x != HTAB_EMPTY_ENTRY && x != HTAB_DELETED_ENTRY) - return x; - }; - return NULL; - } /* Get the number of the next statement uid to be allocated. */ static inline unsigned int --- 42,47 ---- Index: tree-flow.h =================================================================== *** tree-flow.h (revision 202414) --- tree-flow.h (working copy) *************** typedef struct *** 106,118 **** PTR *limit; } htab_iterator; - /* Iterate through the elements of hashtable HTAB, using htab_iterator ITER, - storing each element in RESULT, which is of type TYPE. */ - #define FOR_EACH_HTAB_ELEMENT(HTAB, RESULT, TYPE, ITER) \ - for (RESULT = (TYPE) first_htab_element (&(ITER), (HTAB)); \ - !end_htab_p (&(ITER)); \ - RESULT = (TYPE) next_htab_element (&(ITER))) - /*--------------------------------------------------------------------------- Attributes for SSA_NAMEs. --- 106,111 ----