| Message ID | 1425060448-5315-5-git-send-email-jan.vesely@rutgers.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kristian, this patch partially reverts e4a519635: Tidy up compile warnings by cleaning up types. it looks to me that the xf86drmSL.c bits slipped in by accident. thanks, Jan On Fri, 2015-02-27 at 13:07 -0500, Jan Vesely wrote: > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > --- > xf86drmSL.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xf86drmSL.c b/xf86drmSL.c > index acddb54..2160bb8 100644 > --- a/xf86drmSL.c > +++ b/xf86drmSL.c > @@ -266,11 +266,14 @@ int drmSLLookupNeighbors(void *l, unsigned long key, > SkipListPtr list = (SkipListPtr)l; > SLEntryPtr update[SL_MAX_LEVEL + 1]; > int retcode = 0; > + SLEntryPtr entry; > + > + entry = SLLocate(list, key, update); > > *prev_key = *next_key = key; > *prev_value = *next_value = NULL; > - > - if (update[0]) { > + > + if (entry && update[0]) { > *prev_key = update[0]->key; > *prev_value = update[0]->value; > ++retcode;
On 27/02/15 18:07, Jan Vesely wrote: > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > --- > xf86drmSL.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/xf86drmSL.c b/xf86drmSL.c > index acddb54..2160bb8 100644 > --- a/xf86drmSL.c > +++ b/xf86drmSL.c > @@ -266,11 +266,14 @@ int drmSLLookupNeighbors(void *l, unsigned long key, > SkipListPtr list = (SkipListPtr)l; > SLEntryPtr update[SL_MAX_LEVEL + 1]; > int retcode = 0; > + SLEntryPtr entry; > + > + entry = SLLocate(list, key, update); > > *prev_key = *next_key = key; > *prev_value = *next_value = NULL; > - > - if (update[0]) { > + > + if (entry && update[0]) { From a very brief look at git log, the entry check should not be needed. Must admit that I've not looked at all in the implementation of either SLLocate or drmSLLookupNeighbors. That said it seems that none of the three files (xf86drm{SL,Hash,Random}) has been build as a program for a while. Maybe we could split it out as a standalone test and let it churn at make check time ? Cheers, Emil
On Fri, 2015-03-20 at 17:38 +0000, Emil Velikov wrote: > On 27/02/15 18:07, Jan Vesely wrote: > > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > > --- > > xf86drmSL.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/xf86drmSL.c b/xf86drmSL.c > > index acddb54..2160bb8 100644 > > --- a/xf86drmSL.c > > +++ b/xf86drmSL.c > > @@ -266,11 +266,14 @@ int drmSLLookupNeighbors(void *l, unsigned long key, > > SkipListPtr list = (SkipListPtr)l; > > SLEntryPtr update[SL_MAX_LEVEL + 1]; > > int retcode = 0; > > + SLEntryPtr entry; > > + > > + entry = SLLocate(list, key, update); > > > > *prev_key = *next_key = key; > > *prev_value = *next_value = NULL; > > - > > - if (update[0]) { > > + > > + if (entry && update[0]) { > From a very brief look at git log, the entry check should not be needed. > Must admit that I've not looked at all in the implementation of either > SLLocate or drmSLLookupNeighbors. SLLocate might return early and leave the array uninitialized. All other calls to it check the return value. I guess the warning that the previous commit tried to fix was "set-but-unused" variable. > > That said it seems that none of the three files > (xf86drm{SL,Hash,Random}) has been build as a program for a while. Maybe > we could split it out as a standalone test and let it churn at make > check time ? Sounds like a good idea. I'll try to take a look when time permits, but I'd leave that as a separate patch. thanks, jan > > Cheers, > Emil
On Fri, 2015-03-20 at 14:01 -0400, Jan Vesely wrote: > On Fri, 2015-03-20 at 17:38 +0000, Emil Velikov wrote: > > On 27/02/15 18:07, Jan Vesely wrote: > > > Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> > > > --- > > > xf86drmSL.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/xf86drmSL.c b/xf86drmSL.c > > > index acddb54..2160bb8 100644 > > > --- a/xf86drmSL.c > > > +++ b/xf86drmSL.c > > > @@ -266,11 +266,14 @@ int drmSLLookupNeighbors(void *l, unsigned long key, > > > SkipListPtr list = (SkipListPtr)l; > > > SLEntryPtr update[SL_MAX_LEVEL + 1]; > > > int retcode = 0; > > > + SLEntryPtr entry; > > > + > > > + entry = SLLocate(list, key, update); > > > > > > *prev_key = *next_key = key; > > > *prev_value = *next_value = NULL; > > > - > > > - if (update[0]) { > > > + > > > + if (entry && update[0]) { > > From a very brief look at git log, the entry check should not be needed. > > Must admit that I've not looked at all in the implementation of either > > SLLocate or drmSLLookupNeighbors. > > SLLocate might return early and leave the array uninitialized. All other > calls to it check the return value. I guess the warning that the > previous commit tried to fix was "set-but-unused" variable. I take this back. You were right. SLLocate might return NULL in non-failing case, and we cannot rely on the return value. I'll post an updated patch (and a move to tests) shortly jan > > > > > That said it seems that none of the three files > > (xf86drm{SL,Hash,Random}) has been build as a program for a while. Maybe > > we could split it out as a standalone test and let it churn at make > > check time ? > > Sounds like a good idea. I'll try to take a look when time permits, but > I'd leave that as a separate patch. > > thanks, > jan > > > > > Cheers, > > Emil >
diff --git a/xf86drmSL.c b/xf86drmSL.c index acddb54..2160bb8 100644 --- a/xf86drmSL.c +++ b/xf86drmSL.c @@ -266,11 +266,14 @@ int drmSLLookupNeighbors(void *l, unsigned long key, SkipListPtr list = (SkipListPtr)l; SLEntryPtr update[SL_MAX_LEVEL + 1]; int retcode = 0; + SLEntryPtr entry; + + entry = SLLocate(list, key, update); *prev_key = *next_key = key; *prev_value = *next_value = NULL; - - if (update[0]) { + + if (entry && update[0]) { *prev_key = update[0]->key; *prev_value = update[0]->value; ++retcode;
Signed-off-by: Jan Vesely <jan.vesely@rutgers.edu> --- xf86drmSL.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)