Tautological comparison in gcc 6.1.1

[Amitay Isaacs]

On Wed, Jul 13, 2016 at 8:15 AM, Michael Adam <obnox at samba.org> wrote:

> On 2016-07-12 at 17:32 +0200, Michael Adam wrote:
> > Hi Amitay,
> >
> > I just stumbled across the same issue.
> > Thanks for already having proposed a solution!
> >
> > On 2016-07-06 at 12:26 +1000, Amitay Isaacs wrote:
> > > Hi,
> > >
> > > In gcc 6.1.1 (from fedora 24), there is a new warning for tautological
> > > comparisons.  It issues a warning for DLIST_ADD_END(), so I cannot use
> > > --picky-developer any more.
> > >
> > > Here is the preprocessor output for statement DLIST_ADD_END(list, p)
> > >
> > >  do { if (!(list)) { do { if (!(list)) { (p)->prev = (list) = (p);
> > > (p)->next =
> > > ((void *)0)
> > > ; } else { (p)->prev = (list)->prev; (list)->prev = (p); (p)->next =
> > > (list); (list) = (p); } } while (0); } else { do { if (!(list) ||
> > > !((list)->prev)) { do { if (!(list)) { (p)->prev = (list) = (p);
> (p)->next
> > > =
> > > ((void *)0)
> > > ; } else { (p)->prev = (list)->prev; (list)->prev = (p); (p)->next =
> > > (list); (list) = (p); } } while (0); } else { (p)->prev =
> ((list)->prev);
> > > (p)->next = ((list)->prev)->next; ((list)->prev)->next = (p); if
> > > ((p)->next) (p)->next->prev = (p); if ((list)->prev == ((list)->prev))
> > > (list)->prev = (p); }} while (0); } } while (0);
> > >
> > > In the last else, which is expansion for DLIST_ADD_AFTER(), there is a
> > > tautological comparison.
> > >
> > > if ((list)->prev == ((list)->prev) ...
> > >
> > > This comes from:
> > >
> > > #define DLIST_ADD_AFTER(list, p, el) \
> > > do { \
> > >         if (!(list) || !(el)) { \
> > >         DLIST_ADD(list, p); \
> > >     } else { \
> > >         (p)->prev = (el);   \
> > >         (p)->next = (el)->next;        \
> > >         (el)->next = (p);        \
> > >         if ((p)->next) (p)->next->prev = (p);    \
> > >         if ((list)->prev == (el)) (list)->prev = (p); \
> <-----
> > > culprit
> > >     }\
> > > } while (0)
> > >
> > > One solution would be embed DLIST_ADD_AFTER() code in DLIST_ADD_END()
> and
> > > drop the check in the last statement as follows:
> > >
> > > diff --git a/lib/util/dlinklist.h b/lib/util/dlinklist.h
> > > index 8a1b84d..e969f17 100644
> > > --- a/lib/util/dlinklist.h
> > > +++ b/lib/util/dlinklist.h
> > > @@ -132,8 +132,14 @@ do { \
> > >  do { \
> > >         if (!(list)) { \
> > >                 DLIST_ADD(list, p); \
> > > +       } else if (!((list)->prev)) { \
> > > +               DLIST_ADD(list, p); \
> > >         } else { \
> > > -               DLIST_ADD_AFTER(list, p, (list)->prev); \
> > > +               (p)->prev = (list)->prev; \
> > > +               (p)->next = (list)->prev->next; \
> > > +               (list)->prev->next = (p); \
> > > +               if ((p)->next) (p)->next->prev = (p); \
> > > +               (list)->prev = (p); \
> > >         } \
> > >  } while (0)
> > >
> > >
> > > Is there a better solution?
> >
> > The code duplication instead of re-use lacks elegance,
> > but it is the best solition I can think of, currently
> > (without disabling the Werror flag on all files using
> > the dlist code. Btw, we have additional copies in tevent
> > and ldb that need the same fixing.
>
> Attached are patches that cover all definitions of DLIST_ADD_END
> as above. We could add those with your authorship, amitay.
>

> These are not the only problems encountered with gcc6.
>
> Continuing now ...
>
>
Reviewed-by: me.

Let's wait and see if anyone has a better idea.

Amitay.