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
Jeff King <peff@peff.net>
Date
Jun 15, 2022, 03:48 UTC
Message-ID
<YqlWhKDFX3KESY0h@coredump.intra.peff.net>
In-Reply-To
<Yqk9uMS5kxHD6o7l@nand.local>
On Tue, Jun 14, 2022 at 10:02:32PM -0400, Taylor Blau wrote:
Show 22 quoted lines
> --- >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.

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)
 

-Peff
Previous: Taylor BlauNext: Taylor Blau
Message 7 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.