From: Michael Haggerty <mhagger@alum.mit.edu>
To: "Stefan Beller" <sbeller@google.com>,
"Junio C Hamano" <gitster@pobox.com>,
"SZEDER Gábor" <szeder.dev@gmail.com>,
"Jonathan Nieder" <jrnieder@gmail.com>
Cc: Alex Riesen <raa.lkml@gmail.com>,
"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCH] for_each_string_list_item(): behave correctly for empty list
Date: Tue, 19 Sep 2017 08:51:22 +0200 [thread overview]
Message-ID: <dab2d555-7e09-4eb3-19b8-cab085626bbe@alum.mit.edu> (raw)
In-Reply-To: <CAGZ79kYXDhcVXd2C-x6e=o7jYdKqV22DY45c7E2TeuhKLfn26w@mail.gmail.com>
On 09/19/2017 02:08 AM, Stefan Beller wrote:
>> I am hoping that this last one is not allowed and we can use the
>> "same condition is checked every time we loop" version that hides
>> the uglyness inside the macro.
>
> By which you are referring to Jonathans solution posted.
> Maybe we can combine the two solutions (checking for thelist
> to not be NULL once, by Jonathan) and using an outer structure
> (SZEDERs solution) by replacing the condition by a for loop,
> roughly (untested):
>
> #define for_each_string_list_item(item,list) \
> - for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
> + for (; list; list = NULL)
> + for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
>
> as that would not mingle with any dangling else clause.
> It is also just one statement, such that
>
> if (bla)
> for_each_string_list_item {
> baz(item);
> }
> else
> foo;
>
> still works.
>
> Are there downsides to this combined approach?
On the plus side, it's pleasantly devious; I wouldn't have thought of
using a `for` loop for the initial test. But it doesn't work as written,
because (1) we don't need to guard against `list` being NULL, but rather
`list->items`; and (2) we don't have the liberty to set `list = NULL`
(or `list->items = NULL`, because `list` is owned by the caller and we
shouldn't modify it.
The following is a bit closer:
#define for_each_string_list_item(item,list) \
for (item = (list)->items; item; item = NULL) \
for (; item < (list)->items + (list)->nr; ++item)
But I think that also fails, because a callsite that does
for_each_string_list_item(myitem, mylist)
if (myitem.util)
break;
would expect that `myitem` is still set after breaking out of the loop,
whereas the outer `for` loop would reset it to NULL.
If `break` were an expression we could do something like
#define for_each_string_list_item(item,list) \
for (item = (list)->items; item; break) \
for (; item < (list)->items + (list)->nr; ++item)
So I think we're still left with the suggestions of Jonathan or Gábor.
Or the bigger change of initializing `string_list::items` to point at an
empty sentinal array (similar to `strbuf_slopbuf`) rather than NULL.
Personally, I think that Jonathan's approach makes the most sense,
unless somebody wants to jump in an implement a `string_list_slopbuf`.
By the way, I wonder if any open-coded loops over `string_lists` make
the same mistake as the macro?
Michael
next prev parent reply other threads:[~2017-09-19 6:51 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-15 16:00 [PATCH] for_each_string_list_item(): behave correctly for empty list Michael Haggerty
2017-09-15 18:43 ` Jonathan Nieder
2017-09-16 4:06 ` Michael Haggerty
2017-09-16 11:51 ` SZEDER Gábor
2017-09-17 10:19 ` Michael Haggerty
2017-09-19 14:38 ` Kaartic Sivaraam
2017-09-20 1:38 ` Junio C Hamano
2017-09-20 1:43 ` Jonathan Nieder
2017-09-20 5:14 ` Junio C Hamano
2017-09-20 2:30 ` Jonathan Nieder
2017-09-20 3:54 ` Junio C Hamano
2017-09-20 5:27 ` [PATCH v2] for_each_string_list_item: avoid undefined behavior " Jonathan Nieder
2017-09-20 5:40 ` Junio C Hamano
2017-09-20 7:00 ` Michael Haggerty
2017-09-20 7:40 ` Kaartic Sivaraam
2017-09-20 12:22 ` [PATCH v2] doc: camelCase the config variables to improve readability Kaartic Sivaraam
2017-09-20 16:28 ` [PATCH v2] for_each_string_list_item: avoid undefined behavior for empty list Andreas Schwab
2017-09-20 17:31 ` Jonathan Nieder
2017-09-20 21:51 ` Andreas Schwab
2017-09-21 1:12 ` Junio C Hamano
2017-09-21 15:39 ` Andreas Schwab
2017-09-20 7:35 ` [PATCH] for_each_string_list_item(): behave correctly " Kaartic Sivaraam
2017-09-17 0:59 ` Junio C Hamano
2017-09-17 10:24 ` Michael Haggerty
2017-09-18 0:37 ` Junio C Hamano
2017-09-19 0:08 ` Stefan Beller
2017-09-19 6:51 ` Michael Haggerty [this message]
2017-09-19 13:38 ` SZEDER Gábor
2017-09-19 13:45 ` SZEDER Gábor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dab2d555-7e09-4eb3-19b8-cab085626bbe@alum.mit.edu \
--to=mhagger@alum.mit.edu \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
--cc=raa.lkml@gmail.com \
--cc=sbeller@google.com \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).