[Mesa-dev] [PATCH v3 018/104] nir: Add a deref path helper struct

[Jason Ekstrand]

On Thu, Apr 5, 2018 at 10:06 AM, Caio Marcelo de Oliveira Filho <
caio.oliveira at intel.com> wrote:

> Hello,
>
> > +   if (count <= max_short_path_len) {
> > +      /* If we're under max_short_path_len, just use the short path. */
> > +      path->path = head;
> > +      goto done;
> > +   }
> > +
> > +   path->path = ralloc_array(mem_ctx, nir_deref_instr *, count + 1);
> > +   head = tail = path->path + count;
> > +   *tail = NULL;
> > +   for (nir_deref_instr *d = deref; d; d = nir_deref_instr_parent(d))
> > +      *(--head) = d;
>
> What do you think about zeroing the _short_path when we are not using
> it? I'm guessing cases where we don't use _short_path will not be
> common, so it will help highlight them when debugging.
>

We could.  We could also memset it to 0xdeadbeef in debug builds which is
probably better than NULL.


> > +done:
> > +   assert(head == path->path);
> > +   assert(tail == head + count);
> > +   assert((*head)->deref_type == nir_deref_type_var);
>
> This assert access invalid memory if "deref == NULL", but the rest of
> the code is ready for this case. So I suggest either prefixing this
> assert with "(!*head) || ..." or, if the deref == NULL case is not
> useful/expected, assert(deref) in the beginning of the function.
>

It does.  However, I think it should be an error to call this function with
NULL (I can add an assert) and all of the callers assume that the first
path.path[0] will be a variable dereference so we really do want to check
that.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180405/f9cdf0e1/attachment.html>