[C] field_decl_cmp

Message ID 68b46c41-a333-7273-fe12-ae395d642ccb@acm.org
State New
Headers show
Series
  • [C] field_decl_cmp
Related show

Commit Message

Nathan Sidwell Sept. 12, 2017, 3:25 p.m.
Joseph,
in moving field_decl_cmp to the C FE, I noticed it checks for NULL 
DECL_NAMES.  Those don't occur.

This patch removes that checking, and also asserts that when we see 
identically named decls, exactly one is a TYPE_DECL.

ok?

nathan

Comments

Joseph Myers Sept. 12, 2017, 4:06 p.m. | #1
On Tue, 12 Sep 2017, Nathan Sidwell wrote:

> Joseph,
> in moving field_decl_cmp to the C FE, I noticed it checks for NULL DECL_NAMES.
> Those don't occur.

To be clear: they don't occur in the case where field_decl_cmp is used; 
they can occur in other cases.

> This patch removes that checking, and also asserts that when we see
> identically named decls, exactly one is a TYPE_DECL.

When do you get TYPE_DECLs here, for C?  I wouldn't expect them to be 
possible.
Nathan Sidwell Sept. 12, 2017, 4:41 p.m. | #2
On 09/12/2017 12:06 PM, Joseph Myers wrote:
> On Tue, 12 Sep 2017, Nathan Sidwell wrote:

>> This patch removes that checking, and also asserts that when we see
>> identically named decls, exactly one is a TYPE_DECL.
> 
> When do you get TYPE_DECLs here, for C?  I wouldn't expect them to be
> possible.
>

oh, of course (C is so primitive!).  This patch survives a bootstrap, ok?

nathan
Joseph Myers Sept. 12, 2017, 4:48 p.m. | #3
On Tue, 12 Sep 2017, Nathan Sidwell wrote:

> On 09/12/2017 12:06 PM, Joseph Myers wrote:
> > On Tue, 12 Sep 2017, Nathan Sidwell wrote:
> 
> > > This patch removes that checking, and also asserts that when we see
> > > identically named decls, exactly one is a TYPE_DECL.
> > 
> > When do you get TYPE_DECLs here, for C?  I wouldn't expect them to be
> > possible.
> > 
> 
> oh, of course (C is so primitive!).  This patch survives a bootstrap, ok?

I'd be concerned about the possibility of a qsort implementation that 
calls the comparison function with two pointers to the same object (as far 
as I can tell, it's valid for qsort to do that).  That is, I think you 
need to check for the two DECLs being the same DECL, before asserting 
their names are different.
Nathan Sidwell Sept. 15, 2017, 5:43 p.m. | #4
On 09/12/2017 12:48 PM, Joseph Myers wrote:

> I'd be concerned about the possibility of a qsort implementation that
> calls the comparison function with two pointers to the same object (as far
> as I can tell, it's valid for qsort to do that).  That is, I think you
> need to check for the two DECLs being the same DECL, before asserting
> their names are different.

I suppose we can drop the assert.  That does leave it returning +1 in 
the case you're concerned about, but I don't really see the need to tell 
such a stupid qsort that the things are unordered.

nathan
Joseph Myers Sept. 15, 2017, 5:50 p.m. | #5
On Fri, 15 Sep 2017, Nathan Sidwell wrote:

> On 09/12/2017 12:48 PM, Joseph Myers wrote:
> 
> > I'd be concerned about the possibility of a qsort implementation that
> > calls the comparison function with two pointers to the same object (as far
> > as I can tell, it's valid for qsort to do that).  That is, I think you
> > need to check for the two DECLs being the same DECL, before asserting
> > their names are different.
> 
> I suppose we can drop the assert.  That does leave it returning +1 in the case
> you're concerned about, but I don't really see the need to tell such a stupid
> qsort that the things are unordered.

I don't know what such a qsort would do if such a case returned 1; my 
presumption is that all our comparison functions ought to return 0 when 
two objects are equal, even if that can only be if they are the same 
object.  It's OK with a return of 0 if x == y (or if DECL_NAME (x) == 
DECL_NAME (y), whichever you think appropriate).

Patch

2017-09-12  Nathan Sidwell  <nathan@acm.org>

	* c-decl.c (field_decl_cmp): Don't handle NULL names.  Refactor.

Index: c-decl.c
===================================================================
--- c-decl.c	(revision 252023)
+++ c-decl.c	(working copy)
@@ -7845,19 +7845,17 @@  warn_cxx_compat_finish_struct (tree fiel
 static int
 field_decl_cmp (const void *x_p, const void *y_p)
 {
-  const tree *const x = (const tree *) x_p;
-  const tree *const y = (const tree *) y_p;
+  const tree x = *(const tree *) x_p;
+  const tree y = *(const tree *) y_p;
 
-  if (DECL_NAME (*x) == DECL_NAME (*y))
-    /* A nontype is "greater" than a type.  */
-    return (TREE_CODE (*y) == TYPE_DECL) - (TREE_CODE (*x) == TYPE_DECL);
-  if (DECL_NAME (*x) == NULL_TREE)
-    return -1;
-  if (DECL_NAME (*y) == NULL_TREE)
-    return 1;
-  if (DECL_NAME (*x) < DECL_NAME (*y))
-    return -1;
-  return 1;
+  if (DECL_NAME (x) != DECL_NAME (y))
+    return DECL_NAME (x) < DECL_NAME (y) ? -1 : +1;
+
+  /* If the names are the same, exactly one must be a TYPE_DECL, and
+     that one is less than (before) the other one.  */
+  gcc_checking_assert ((TREE_CODE (x) == TYPE_DECL)
+		       != (TREE_CODE (y) == TYPE_DECL));
+  return TREE_CODE (x) == TYPE_DECL ? -1 : +1;
 }
 
 /* Fill in the fields of a RECORD_TYPE or UNION_TYPE node, T.