Patchwork Remove unused macro and functions from tree-flow.h and tree-flow-inline.h

login
register
mail settings
Submitter Andrew MacLeod
Date Sept. 10, 2013, 12:59 a.m.
Message ID <522E6EFE.2080602@redhat.com>
Download mbox | patch
Permalink /patch/273737/
State New
Headers show

Comments

Andrew MacLeod - Sept. 10, 2013, 12:59 a.m.
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?

Andrew
Jeff Law - Sept. 10, 2013, 2:39 a.m.
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
Jakub Jelinek - Sept. 10, 2013, 5:27 a.m.
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
Andrew MacLeod - Sept. 10, 2013, 11:42 a.m.
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
Jakub Jelinek - Sept. 10, 2013, 12:30 p.m.
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
Andrew MacLeod - Sept. 10, 2013, 1:04 p.m.
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
Jakub Jelinek - Sept. 10, 2013, 1:15 p.m.
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
Andrew MacLeod - Sept. 10, 2013, 1:46 p.m.
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
Tom Tromey - Sept. 10, 2013, 2:42 p.m.
>>>>> "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

Patch


	* 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 ----