git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable

From
Taylor Blau <me@ttaylorr.com>
Date
Jun 15, 2022, 20:47 UTC
Message-ID
<YqpFXoKPu2E1j+mr@nand.local>
In-Reply-To
<YqlWhKDFX3KESY0h@coredump.intra.peff.net>
On Tue, Jun 14, 2022 at 11:48:20PM -0400, Jeff King wrote:
Show 30 quoted lines
> On Tue, Jun 14, 2022 at 10:02:32PM -0400, Taylor Blau wrote:
>
> > --- >8 ---
> >
> > diff --git a/string-list.h b/string-list.h
> > index d5a744e143..425abc55f4 100644
> > --- a/string-list.h
> > +++ b/string-list.h
> > @@ -143,7 +143,7 @@ int for_each_string_list(struct string_list *list,
> >
> >  /** Iterate over each item, as a macro. */
> >  #define for_each_string_list_item(item,list)            \
> > -	for (item = (list)->items;                      \
> > +	for (item = (list) ? (list)->items : NULL;      \
> >  	     item && item < (list)->items + (list)->nr; \
> >  	     ++item)
> >
> > --- 8< ---
> >
> > > but even with your suggestion, I get this compiler error:
> >
> > ...so did I. Though I'm not sure I understand the compiler's warning
> > here. Surely the thing being passed as list in the macro expansion
> > _won't_ always evaluate to non-NULL, will it?
>
> In the general case, no, but in this specific expansion of the macro, it
> is passing the address of a local variable (&cpath), which will never be
> NULL. The compiler is overeager here; the check is indeed pointless in
> this expansion, but warning on useless macro-expanded code isn't
> helpful, since other macro users need it.

Ah, that makes sense. The compiler is warning us that the macro-expanded version of for_each_string_list_item() has a ternary expression that will never evaluate its right-hand side in cases where it can prove the second argument to the macro is non-NULL.

Show 21 quoted lines
> Hiding it in a function seems to work, even with -O2 inlining, like:
>
> diff --git a/string-list.h b/string-list.h
> index d5a744e143..b28b135e11 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -141,9 +141,14 @@ void string_list_clear_func(struct string_list *list, string_list_clear_func_t c
>  int for_each_string_list(struct string_list *list,
>  			 string_list_each_func_t func, void *cb_data);
>
> +static inline struct string_list_item *string_list_first_item(const struct string_list *list)
> +{
> +	return list ? list->items : NULL;
> +}
> +
>  /** Iterate over each item, as a macro. */
>  #define for_each_string_list_item(item,list)            \
> -	for (item = (list)->items;                      \
> +	for (item = string_list_first_item(list);       \
>  	     item && item < (list)->items + (list)->nr; \
>  	     ++item)

That works, nice. I don't really want to mess up the tree too much this close to a release, but this sort of clean-up seems good to do. I know Stolee identified a handful of spots that would benefit from it. Some good #leftoverbits, I guess :-).

Thanks, Taylor

Previous: Jeff KingNext: Junio C Hamano
Message 8 of 14 in “revision: mark blobs needed for resolve-undo as reachable”
  1. revision: mark blobs needed for resolve-undo as reachableJunio C Hamano, Jun 9, 2022
  2. Derrick StoleeJun 13, 2022
  3. Junio C HamanoJun 13, 2022
  4. Ævar Arnfjörð BjarmasonJun 14, 2022
  5. Derrick StoleeJun 14, 2022
  6. Taylor BlauJun 15, 2022
  7. Jeff KingJun 15, 2022
  8. Taylor BlauJun 15, 2022
  9. Junio C HamanoJun 15, 2022
  10. Ævar Arnfjörð BjarmasonJun 16, 2022
  11. Taylor BlauJun 14, 2022
  12. fsck segfault (was: Re: [PATCH] revision: mark blobs needed for resolve-undo as reachable)SZEDER Gábor, Jul 11, 2022
  13. Junio C HamanoJul 11, 2022
  14. 2/1 fsck: do not dereference NULL while checking resolve-undo dataJunio C Hamano, Jul 11, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.