<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Apr 5, 2018 at 10:06 AM, Caio Marcelo de Oliveira Filho <span dir="ltr"><<a href="mailto:caio.oliveira@intel.com" target="_blank">caio.oliveira@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello,<br>
<span class=""><br>
> +   if (count <= max_short_path_len) {<br>
> +      /* If we're under max_short_path_len, just use the short path. */<br>
> +      path->path = head;<br>
> +      goto done;<br>
> +   }<br>
> +<br>
> +   path->path = ralloc_array(mem_ctx, nir_deref_instr *, count + 1);<br>
> +   head = tail = path->path + count;<br>
> +   *tail = NULL;<br>
> +   for (nir_deref_instr *d = deref; d; d = nir_deref_instr_parent(d))<br>
> +      *(--head) = d;<br>
<br>
</span>What do you think about zeroing the _short_path when we are not using<br>
it? I'm guessing cases where we don't use _short_path will not be<br>
common, so it will help highlight them when debugging.<span class=""><br></span></blockquote><div><br></div><div>We could.  We could also memset it to 0xdeadbeef in debug builds which is probably better than NULL.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> +done:<br>
> +   assert(head == path->path);<br>
> +   assert(tail == head + count);<br>
> +   assert((*head)->deref_type == nir_deref_type_var);<br>
<br>
</span>This assert access invalid memory if "deref == NULL", but the rest of<br>
the code is ready for this case. So I suggest either prefixing this<br>
assert with "(!*head) || ..." or, if the deref == NULL case is not<br>
useful/expected, assert(deref) in the beginning of the function.<br>
</blockquote></div><br></div><div class="gmail_extra">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.<br></div></div>