{"thread":{"id":"61257","subject":"[RFC][PATCH] t-prio-queue: simplify using compound literals","startedAt":"2024-04-02T18:30:34Z","lastAt":"2024-04-11T21:24:02Z","messageCount":11,"participants":["René Scharfe","Junio C Hamano","Jeff King","Josh Steadmon"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492096","messageId":"520da361-1b80-4ba3-87b2-86d6fdfc18b5@web.de","threadId":"61257","inReplyTo":null,"subject":"[RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-02T18:30:31Z","receivedAt":"2024-04-02T18:30:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Test names like \"basic\" are mentioned seven times in the code (ignoring\ncase): Twice when defining the input and result macros, thrice when\ndefining the test function, and twice again when calling it.  Reduce\nthat to a single time by using compound literals to pass the input and\nresult arrays via TEST_INPUT to test_prio_queue().\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nC99 added compound literals.  Are we ready to use them?\n\nTest definitions become more compact, but look busier due to the added\npunctuation.  We could hide some of it with a sugary macro like this:\n#define INT_ARRAY(...) ((int []){ __VA_ARGS__ })\n\n t/unit-tests/t-prio-queue.c | 51 +++++++++++++------------------------\n 1 file changed, 17 insertions(+), 34 deletions(-)\n\ndiff --git a/t/unit-tests/t-prio-queue.c b/t/unit-tests/t-prio-queue.c\nindex 5358346361..7a4e5780e1 100644\n--- a/t/unit-tests/t-prio-queue.c\n+++ b/t/unit-tests/t-prio-queue.c\n@@ -66,43 +66,26 @@ static void test_prio_queue(int *input, size_t input_size,\n \tclear_prio_queue(&pq);\n }\n\n-#define BASIC_INPUT 2, 6, 3, 10, 9, 5, 7, 4, 5, 8, 1, DUMP\n-#define BASIC_RESULT 1, 2, 3, 4, 5, 5, 6, 7, 8, 9, 10\n-\n-#define MIXED_PUT_GET_INPUT 6, 2, 4, GET, 5, 3, GET, GET, 1, DUMP\n-#define MIXED_PUT_GET_RESULT 2, 3, 4, 1, 5, 6\n-\n-#define EMPTY_QUEUE_INPUT 1, 2, GET, GET, GET, 1, 2, GET, GET, GET\n-#define EMPTY_QUEUE_RESULT 1, 2, MISSING, 1, 2, MISSING\n-\n-#define STACK_INPUT STACK, 8, 1, 5, 4, 6, 2, 3, DUMP\n-#define STACK_RESULT 3, 2, 6, 4, 5, 1, 8\n-\n-#define REVERSE_STACK_INPUT STACK, 1, 2, 3, 4, 5, 6, REVERSE, DUMP\n-#define REVERSE_STACK_RESULT 1, 2, 3, 4, 5, 6\n-\n-#define TEST_INPUT(INPUT, RESULT, name)\t\t\t\\\n-  static void test_##name(void)\t\t\t\t\\\n-{\t\t\t\t\t\t\t\t\\\n-\tint input[] = {INPUT};\t\t\t\t\t\\\n-\tint result[] = {RESULT};\t\t\t\t\\\n-\ttest_prio_queue(input, ARRAY_SIZE(input),\t\t\\\n-\t\t\tresult, ARRAY_SIZE(result));\t\t\\\n-}\n-\n-TEST_INPUT(BASIC_INPUT, BASIC_RESULT, basic)\n-TEST_INPUT(MIXED_PUT_GET_INPUT, MIXED_PUT_GET_RESULT, mixed)\n-TEST_INPUT(EMPTY_QUEUE_INPUT, EMPTY_QUEUE_RESULT, empty)\n-TEST_INPUT(STACK_INPUT, STACK_RESULT, stack)\n-TEST_INPUT(REVERSE_STACK_INPUT, REVERSE_STACK_RESULT, reverse)\n+#define TEST_INPUT(input, result) \\\n+\ttest_prio_queue(input, ARRAY_SIZE(input), result, ARRAY_SIZE(result))\n\n int cmd_main(int argc, const char **argv)\n {\n-\tTEST(test_basic(), \"prio-queue works for basic input\");\n-\tTEST(test_mixed(), \"prio-queue works for mixed put & get commands\");\n-\tTEST(test_empty(), \"prio-queue works when queue is empty\");\n-\tTEST(test_stack(), \"prio-queue works when used as a LIFO stack\");\n-\tTEST(test_reverse(), \"prio-queue works when LIFO stack is reversed\");\n+\tTEST(TEST_INPUT(((int []){ 2, 6, 3, 10, 9, 5, 7, 4, 5, 8, 1, DUMP }),\n+\t\t\t((int []){ 1, 2, 3, 4, 5, 5, 6, 7, 8, 9, 10 })),\n+\t     \"prio-queue works for basic input\");\n+\tTEST(TEST_INPUT(((int []){ 6, 2, 4, GET, 5, 3, GET, GET, 1, DUMP }),\n+\t\t\t((int []){ 2, 3, 4, 1, 5, 6 })),\n+\t     \"prio-queue works for mixed put & get commands\");\n+\tTEST(TEST_INPUT(((int []){ 1, 2, GET, GET, GET, 1, 2, GET, GET, GET }),\n+\t\t\t((int []){ 1, 2, MISSING, 1, 2, MISSING })),\n+\t     \"prio-queue works when queue is empty\");\n+\tTEST(TEST_INPUT(((int []){ STACK, 8, 1, 5, 4, 6, 2, 3, DUMP }),\n+\t\t\t((int []){ 3, 2, 6, 4, 5, 1, 8 })),\n+\t     \"prio-queue works when used as a LIFO stack\");\n+\tTEST(TEST_INPUT(((int []){ STACK, 1, 2, 3, 4, 5, 6, REVERSE, DUMP }),\n+\t\t\t((int []){ 1, 2, 3, 4, 5, 6 })),\n+\t     \"prio-queue works when LIFO stack is reversed\");\n\n \treturn test_done();\n }\n--\n2.44.0\n"},{"id":"492110","messageId":"xmqq7chf4id4.fsf@gitster.g","threadId":"61257","inReplyTo":"520da361-1b80-4ba3-87b2-86d6fdfc18b5@web.de","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-02T20:31:03Z","receivedAt":"2024-04-02T20:31:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Test names like \"basic\" are mentioned seven times in the code (ignoring\n> case): Twice when defining the input and result macros, thrice when\n> defining the test function, and twice again when calling it.  Reduce\n> that to a single time by using compound literals to pass the input and\n> result arrays via TEST_INPUT to test_prio_queue().\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> C99 added compound literals.  Are we ready to use them?\n\nWe don't know.  This might be a good weather-baloon, but I do not\nknow if people skip t/unit-tests/ without telling us.  After a few\nreleases with this patch in, perhaps we can find a more central\nplace to use them to deliberately break the build of these folks and\nhave them complain, and revert it if it becomes needed?\n\n"},{"id":"492112","messageId":"20240402204153.GE875182@coredump.intra.peff.net","threadId":"61257","inReplyTo":"520da361-1b80-4ba3-87b2-86d6fdfc18b5@web.de","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-02T20:41:53Z","receivedAt":"2024-04-02T20:41:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 02, 2024 at 08:30:31PM +0200, René Scharfe wrote:\n\n> Test names like \"basic\" are mentioned seven times in the code (ignoring\n> case): Twice when defining the input and result macros, thrice when\n> defining the test function, and twice again when calling it.  Reduce\n> that to a single time by using compound literals to pass the input and\n> result arrays via TEST_INPUT to test_prio_queue().\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> C99 added compound literals.  Are we ready to use them?\n> \n> Test definitions become more compact, but look busier due to the added\n> punctuation.  We could hide some of it with a sugary macro like this:\n> #define INT_ARRAY(...) ((int []){ __VA_ARGS__ })\n\nFWIW, I think the result is way more readable than the original, even\nwithout the INT_ARRAY (which I'm not sure adds much). I don't have a\nstrong opinion on compound literals in general, but if we did allow\nthem, we could clean up the horrible non-reentrant DATE_MODE().\n\n-Peff\n"},{"id":"492310","messageId":"c6cb255a-72f0-4ac2-81a2-1d8e95570a81@web.de","threadId":"61257","inReplyTo":"20240402204153.GE875182@coredump.intra.peff.net","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-05T17:44:59Z","receivedAt":"2024-04-05T17:45:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.04.24 um 22:41 schrieb Jeff King:\n>\n> I don't have a\n> strong opinion on compound literals in general, but if we did allow\n> them, we could clean up the horrible non-reentrant DATE_MODE().\n\nWe can easily make them reentrant without compound literals.  It just\nrequires simple changes to lots of places.  Patch below.\n\n--- >8 ---\nSubject: [PATCH] date: make DATE_MODE thread-safe\n\ndate_mode_from_type() modifies a static variable and returns a pointer\nto it.  This is not thread-safe.  Most callers of date_mode_from_type()\nuse it via the macro DATE_MODE and pass its result on to functions like\nshow_date(), which take a const pointer and don't modify the struct.\n\nAvoid the static storage by putting the variable on the stack and\nreturning the whole struct date_mode.  Change functions that take a\nconstant pointer to expect the whole struct instead.\n\nReduce the cost of passing struct date_mode around on 64-bit systems\nby reordering its members to close the hole between the 32-bit wide\n.type and the 64-bit aligned .strftime_fmt as well as the alignment\nhole at the end.  sizeof reports 24 before and 16 with this change\non x64.  Keep .type at the top to still allow initialization without\ndesignator -- though that's only done in a single location, in\nbuiltin/blame.c.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/blame.c      |  4 ++--\n date.c               | 36 ++++++++++++++++++------------------\n date.h               |  6 +++---\n gpg-interface.c      |  2 +-\n log-tree.c           |  2 +-\n oss-fuzz/fuzz-date.c |  6 +++---\n pretty.c             | 18 +++++++++---------\n pretty.h             |  2 +-\n ref-filter.c         |  2 +-\n reflog-walk.c        |  4 ++--\n reflog-walk.h        |  4 ++--\n t/helper/test-date.c |  2 +-\n 12 files changed, 44 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex db1f56de61..9aa74680a3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -316,7 +316,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,\n \t\tsize_t time_width;\n \t\tint tz;\n \t\ttz = atoi(tz_str);\n-\t\ttime_str = show_date(time, tz, &blame_date_mode);\n+\t\ttime_str = show_date(time, tz, blame_date_mode);\n \t\tstrbuf_addstr(&time_buf, time_str);\n \t\t/*\n \t\t * Add space paddings to time_buf to display a fixed width\n@@ -1029,7 +1029,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\tblame_date_width = sizeof(\"Thu Oct 19 16:00:04 2006 -0700\");\n \t\tbreak;\n \tcase DATE_STRFTIME:\n-\t\tblame_date_width = strlen(show_date(0, 0, &blame_date_mode)) + 1; /* add the null */\n+\t\tblame_date_width = strlen(show_date(0, 0, blame_date_mode)) + 1; /* add the null */\n \t\tbreak;\n \t}\n \tblame_date_width -= 1; /* strip the null */\ndiff --git a/date.c b/date.c\nindex 44cf2221d8..7365a4ad24 100644\n--- a/date.c\n+++ b/date.c\n@@ -207,13 +207,13 @@ void show_date_relative(timestamp_t time, struct strbuf *timebuf)\n \t\t (diff + 183) / 365);\n }\n\n-struct date_mode *date_mode_from_type(enum date_mode_type type)\n+struct date_mode date_mode_from_type(enum date_mode_type type)\n {\n-\tstatic struct date_mode mode = DATE_MODE_INIT;\n+\tstruct date_mode mode = DATE_MODE_INIT;\n \tif (type == DATE_STRFTIME)\n \t\tBUG(\"cannot create anonymous strftime date_mode struct\");\n \tmode.type = type;\n-\treturn &mode;\n+\treturn mode;\n }\n\n static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm, int tz, struct tm *human_tm, int human_tz, int local)\n@@ -283,7 +283,7 @@ static void show_date_normal(struct strbuf *buf, timestamp_t time, struct tm *tm\n \t\tstrbuf_addf(buf, \" %+05d\", tz);\n }\n\n-const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n+const char *show_date(timestamp_t time, int tz, struct date_mode mode)\n {\n \tstruct tm *tm;\n \tstruct tm tmbuf = { 0 };\n@@ -291,13 +291,13 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \tint human_tz = -1;\n \tstatic struct strbuf timebuf = STRBUF_INIT;\n\n-\tif (mode->type == DATE_UNIX) {\n+\tif (mode.type == DATE_UNIX) {\n \t\tstrbuf_reset(&timebuf);\n \t\tstrbuf_addf(&timebuf, \"%\"PRItime, time);\n \t\treturn timebuf.buf;\n \t}\n\n-\tif (mode->type == DATE_HUMAN) {\n+\tif (mode.type == DATE_HUMAN) {\n \t\tstruct timeval now;\n\n \t\tget_time(&now);\n@@ -306,22 +306,22 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \t\thuman_tz = local_time_tzoffset(now.tv_sec, &human_tm);\n \t}\n\n-\tif (mode->local)\n+\tif (mode.local)\n \t\ttz = local_tzoffset(time);\n\n-\tif (mode->type == DATE_RAW) {\n+\tif (mode.type == DATE_RAW) {\n \t\tstrbuf_reset(&timebuf);\n \t\tstrbuf_addf(&timebuf, \"%\"PRItime\" %+05d\", time, tz);\n \t\treturn timebuf.buf;\n \t}\n\n-\tif (mode->type == DATE_RELATIVE) {\n+\tif (mode.type == DATE_RELATIVE) {\n \t\tstrbuf_reset(&timebuf);\n \t\tshow_date_relative(time, &timebuf);\n \t\treturn timebuf.buf;\n \t}\n\n-\tif (mode->local)\n+\tif (mode.local)\n \t\ttm = time_to_tm_local(time, &tmbuf);\n \telse\n \t\ttm = time_to_tm(time, tz, &tmbuf);\n@@ -331,17 +331,17 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \t}\n\n \tstrbuf_reset(&timebuf);\n-\tif (mode->type == DATE_SHORT)\n+\tif (mode.type == DATE_SHORT)\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02d\", tm->tm_year + 1900,\n \t\t\t\ttm->tm_mon + 1, tm->tm_mday);\n-\telse if (mode->type == DATE_ISO8601)\n+\telse if (mode.type == DATE_ISO8601)\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02d %02d:%02d:%02d %+05d\",\n \t\t\t\ttm->tm_year + 1900,\n \t\t\t\ttm->tm_mon + 1,\n \t\t\t\ttm->tm_mday,\n \t\t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec,\n \t\t\t\ttz);\n-\telse if (mode->type == DATE_ISO8601_STRICT) {\n+\telse if (mode.type == DATE_ISO8601_STRICT) {\n \t\tstrbuf_addf(&timebuf, \"%04d-%02d-%02dT%02d:%02d:%02d\",\n \t\t\t\ttm->tm_year + 1900,\n \t\t\t\ttm->tm_mon + 1,\n@@ -354,16 +354,16 @@ const char *show_date(timestamp_t time, int tz, const struct date_mode *mode)\n \t\t\ttz = abs(tz);\n \t\t\tstrbuf_addf(&timebuf, \"%02d:%02d\", tz / 100, tz % 100);\n \t\t}\n-\t} else if (mode->type == DATE_RFC2822)\n+\t} else if (mode.type == DATE_RFC2822)\n \t\tstrbuf_addf(&timebuf, \"%.3s, %d %.3s %d %02d:%02d:%02d %+05d\",\n \t\t\tweekday_names[tm->tm_wday], tm->tm_mday,\n \t\t\tmonth_names[tm->tm_mon], tm->tm_year + 1900,\n \t\t\ttm->tm_hour, tm->tm_min, tm->tm_sec, tz);\n-\telse if (mode->type == DATE_STRFTIME)\n-\t\tstrbuf_addftime(&timebuf, mode->strftime_fmt, tm, tz,\n-\t\t\t\t!mode->local);\n+\telse if (mode.type == DATE_STRFTIME)\n+\t\tstrbuf_addftime(&timebuf, mode.strftime_fmt, tm, tz,\n+\t\t\t\t!mode.local);\n \telse\n-\t\tshow_date_normal(&timebuf, time, tm, tz, &human_tm, human_tz, mode->local);\n+\t\tshow_date_normal(&timebuf, time, tm, tz, &human_tm, human_tz, mode.local);\n \treturn timebuf.buf;\n }\n\ndiff --git a/date.h b/date.h\nindex 6136212a19..0747864fd7 100644\n--- a/date.h\n+++ b/date.h\n@@ -22,8 +22,8 @@ enum date_mode_type {\n\n struct date_mode {\n \tenum date_mode_type type;\n-\tconst char *strftime_fmt;\n \tint local;\n+\tconst char *strftime_fmt;\n };\n\n #define DATE_MODE_INIT { \\\n@@ -36,14 +36,14 @@ struct date_mode {\n  *   show_date(t, tz, DATE_MODE(NORMAL));\n  */\n #define DATE_MODE(t) date_mode_from_type(DATE_##t)\n-struct date_mode *date_mode_from_type(enum date_mode_type type);\n+struct date_mode date_mode_from_type(enum date_mode_type type);\n\n /**\n  * Format <'time', 'timezone'> into static memory according to 'mode'\n  * and return it. The mode is an initialized \"struct date_mode\"\n  * (usually from the DATE_MODE() macro).\n  */\n-const char *show_date(timestamp_t time, int timezone, const struct date_mode *mode);\n+const char *show_date(timestamp_t time, int timezone, struct date_mode mode);\n\n /**\n  * Parse a date format for later use with show_date().\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 95e764acb1..1ee2c94a3b 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -483,7 +483,7 @@ static int verify_ssh_signed_buffer(struct signature_check *sigc,\n\n \tif (sigc->payload_timestamp)\n \t\tstrbuf_addf(&verify_time, \"-Overify-time=%s\",\n-\t\t\tshow_date(sigc->payload_timestamp, 0, &verify_date_mode));\n+\t\t\tshow_date(sigc->payload_timestamp, 0, verify_date_mode));\n\n \t/* Find the principal from the signers */\n \tstrvec_pushl(&ssh_keygen.args, fmt->program,\ndiff --git a/log-tree.c b/log-tree.c\nindex 59eeaef1f7..16031b44e7 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -773,7 +773,7 @@ void show_log(struct rev_info *opt)\n \t\t\t */\n \t\t\tshow_reflog_message(opt->reflog_info,\n \t\t\t\t\t    opt->commit_format == CMIT_FMT_ONELINE,\n-\t\t\t\t\t    &opt->date_mode,\n+\t\t\t\t\t    opt->date_mode,\n \t\t\t\t\t    opt->date_mode_explicit);\n \t\t\tif (opt->commit_format == CMIT_FMT_ONELINE)\n \t\t\t\treturn;\ndiff --git a/oss-fuzz/fuzz-date.c b/oss-fuzz/fuzz-date.c\nindex 036378b946..9619dae40e 100644\n--- a/oss-fuzz/fuzz-date.c\n+++ b/oss-fuzz/fuzz-date.c\n@@ -11,7 +11,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tint16_t tz;\n \ttimestamp_t ts;\n \tenum date_mode_type dmtype;\n-\tstruct date_mode *dm;\n+\tstruct date_mode dm;\n\n \tif (size <= 4)\n \t\t/*\n@@ -40,10 +40,10 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tfree(str);\n\n \tdm = date_mode_from_type(dmtype);\n-\tdm->local = local;\n+\tdm.local = local;\n \tshow_date(ts, (int)tz, dm);\n\n-\tdate_mode_release(dm);\n+\tdate_mode_release(&dm);\n\n \treturn 0;\n }\ndiff --git a/pretty.c b/pretty.c\nindex 2faf651d3e..7ead078998 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -428,7 +428,7 @@ static void add_rfc2047(struct strbuf *sb, const char *line, size_t len,\n }\n\n const char *show_ident_date(const struct ident_split *ident,\n-\t\t\t    const struct date_mode *mode)\n+\t\t\t    struct date_mode mode)\n {\n \ttimestamp_t date = 0;\n \tlong tz = 0;\n@@ -592,7 +592,7 @@ void pp_user_info(struct pretty_print_context *pp,\n \tswitch (pp->fmt) {\n \tcase CMIT_FMT_MEDIUM:\n \t\tstrbuf_addf(sb, \"Date:   %s\\n\",\n-\t\t\t    show_ident_date(&ident, &pp->date_mode));\n+\t\t\t    show_ident_date(&ident, pp->date_mode));\n \t\tbreak;\n \tcase CMIT_FMT_EMAIL:\n \tcase CMIT_FMT_MBOXRD:\n@@ -601,7 +601,7 @@ void pp_user_info(struct pretty_print_context *pp,\n \t\tbreak;\n \tcase CMIT_FMT_FULLER:\n \t\tstrbuf_addf(sb, \"%sDate: %s\\n\", what,\n-\t\t\t    show_ident_date(&ident, &pp->date_mode));\n+\t\t\t    show_ident_date(&ident, pp->date_mode));\n \t\tbreak;\n \tdefault:\n \t\t/* notin' */\n@@ -775,7 +775,7 @@ static int mailmap_name(const char **email, size_t *email_len,\n\n static size_t format_person_part(struct strbuf *sb, char part,\n \t\t\t\t const char *msg, int len,\n-\t\t\t\t const struct date_mode *dmode)\n+\t\t\t\t struct date_mode dmode)\n {\n \t/* currently all placeholders have same length */\n \tconst int placeholder_len = 2;\n@@ -1034,7 +1034,7 @@ static void rewrap_message_tail(struct strbuf *sb,\n static int format_reflog_person(struct strbuf *sb,\n \t\t\t\tchar part,\n \t\t\t\tstruct reflog_walk_info *log,\n-\t\t\t\tconst struct date_mode *dmode)\n+\t\t\t\tstruct date_mode dmode)\n {\n \tconst char *ident;\n\n@@ -1602,7 +1602,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\tif (c->pretty_ctx->reflog_info)\n \t\t\t\tget_reflog_selector(sb,\n \t\t\t\t\t\t    c->pretty_ctx->reflog_info,\n-\t\t\t\t\t\t    &c->pretty_ctx->date_mode,\n+\t\t\t\t\t\t    c->pretty_ctx->date_mode,\n \t\t\t\t\t\t    c->pretty_ctx->date_mode_explicit,\n \t\t\t\t\t\t    (placeholder[1] == 'd'));\n \t\t\treturn 2;\n@@ -1617,7 +1617,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \t\t\treturn format_reflog_person(sb,\n \t\t\t\t\t\t    placeholder[1],\n \t\t\t\t\t\t    c->pretty_ctx->reflog_info,\n-\t\t\t\t\t\t    &c->pretty_ctx->date_mode);\n+\t\t\t\t\t\t    c->pretty_ctx->date_mode);\n \t\t}\n \t\treturn 0;\t/* unknown %g placeholder */\n \tcase 'N':\n@@ -1712,11 +1712,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tcase 'a':\t/* author ... */\n \t\treturn format_person_part(sb, placeholder[1],\n \t\t\t\t   msg + c->author.off, c->author.len,\n-\t\t\t\t   &c->pretty_ctx->date_mode);\n+\t\t\t\t   c->pretty_ctx->date_mode);\n \tcase 'c':\t/* committer ... */\n \t\treturn format_person_part(sb, placeholder[1],\n \t\t\t\t   msg + c->committer.off, c->committer.len,\n-\t\t\t\t   &c->pretty_ctx->date_mode);\n+\t\t\t\t   c->pretty_ctx->date_mode);\n \tcase 'e':\t/* encoding */\n \t\tif (c->commit_encoding)\n \t\t\tstrbuf_addstr(sb, c->commit_encoding);\ndiff --git a/pretty.h b/pretty.h\nindex 9cc9e5d42b..df267afe4a 100644\n--- a/pretty.h\n+++ b/pretty.h\n@@ -167,7 +167,7 @@ int format_set_trailers_options(struct process_trailer_options *opts,\n  * a well-known sentinel date if they appear bogus.\n  */\n const char *show_ident_date(const struct ident_split *id,\n-\t\t\t    const struct date_mode *mode);\n+\t\t\t    struct date_mode mode);\n\n\n #endif /* PRETTY_H */\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 03542d0236..59ad6f54dd 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1627,7 +1627,7 @@ static void grab_date(const char *buf, struct atom_value *v, const char *atomnam\n \ttz = strtol(zone, NULL, 10);\n \tif ((tz == LONG_MIN || tz == LONG_MAX) && errno == ERANGE)\n \t\tgoto bad;\n-\tv->s = xstrdup(show_date(timestamp, tz, &date_mode));\n+\tv->s = xstrdup(show_date(timestamp, tz, date_mode));\n \tv->value = timestamp;\n \tdate_mode_release(&date_mode);\n \treturn;\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex d216f6f966..66484f4f32 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -223,7 +223,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n\n void get_reflog_selector(struct strbuf *sb,\n \t\t\t struct reflog_walk_info *reflog_info,\n-\t\t\t const struct date_mode *dmode, int force_date,\n+\t\t\t struct date_mode dmode, int force_date,\n \t\t\t int shorten)\n {\n \tstruct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;\n@@ -297,7 +297,7 @@ timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info)\n }\n\n void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,\n-\t\t\t const struct date_mode *dmode, int force_date)\n+\t\t\t struct date_mode dmode, int force_date)\n {\n \tif (reflog_info && reflog_info->last_commit_reflog) {\n \t\tstruct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;\ndiff --git a/reflog-walk.h b/reflog-walk.h\nindex 4d93a26957..989583dc55 100644\n--- a/reflog-walk.h\n+++ b/reflog-walk.h\n@@ -10,14 +10,14 @@ void reflog_walk_info_release(struct reflog_walk_info *info);\n int add_reflog_for_walk(struct reflog_walk_info *info,\n \t\t\tstruct commit *commit, const char *name);\n void show_reflog_message(struct reflog_walk_info *info, int,\n-\t\t\t const struct date_mode *, int force_date);\n+\t\t\t struct date_mode, int force_date);\n void get_reflog_message(struct strbuf *sb,\n \t\t\tstruct reflog_walk_info *reflog_info);\n const char *get_reflog_ident(struct reflog_walk_info *reflog_info);\n timestamp_t get_reflog_timestamp(struct reflog_walk_info *reflog_info);\n void get_reflog_selector(struct strbuf *sb,\n \t\t\t struct reflog_walk_info *reflog_info,\n-\t\t\t const struct date_mode *dmode, int force_date,\n+\t\t\t struct date_mode dmode, int force_date,\n \t\t\t int shorten);\n\n int reflog_walk_empty(struct reflog_walk_info *walk);\ndiff --git a/t/helper/test-date.c b/t/helper/test-date.c\nindex 0683d46574..f25512de9a 100644\n--- a/t/helper/test-date.c\n+++ b/t/helper/test-date.c\n@@ -52,7 +52,7 @@ static void show_dates(const char **argv, const char *format)\n \t\t\targ++;\n \t\ttz = atoi(arg);\n\n-\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, tz, &mode));\n+\t\tprintf(\"%s -> %s\\n\", *argv, show_date(t, tz, mode));\n \t}\n\n \tdate_mode_release(&mode);\n--\n2.44.0\n"},{"id":"492316","messageId":"20240405191714.GA2561807@coredump.intra.peff.net","threadId":"61257","inReplyTo":"c6cb255a-72f0-4ac2-81a2-1d8e95570a81@web.de","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-05T19:17:14Z","receivedAt":"2024-04-05T19:17:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 07:44:59PM +0200, René Scharfe wrote:\n\n> Am 02.04.24 um 22:41 schrieb Jeff King:\n> >\n> > I don't have a\n> > strong opinion on compound literals in general, but if we did allow\n> > them, we could clean up the horrible non-reentrant DATE_MODE().\n> \n> We can easily make them reentrant without compound literals.  It just\n> requires simple changes to lots of places.  Patch below.\n> \n> --- >8 ---\n> Subject: [PATCH] date: make DATE_MODE thread-safe\n> \n> date_mode_from_type() modifies a static variable and returns a pointer\n> to it.  This is not thread-safe.  Most callers of date_mode_from_type()\n> use it via the macro DATE_MODE and pass its result on to functions like\n> show_date(), which take a const pointer and don't modify the struct.\n> \n> Avoid the static storage by putting the variable on the stack and\n> returning the whole struct date_mode.  Change functions that take a\n> constant pointer to expect the whole struct instead.\n\nYeah, this seems pretty reasonable. I think we've traditionally been\nhesitant to pass or return structs by value, but that's mostly\nsuperstition. IIRC it was illegal before C89, but that's long enough ago\nthat we can presumably ignore it.\n\n-Peff\n"},{"id":"492326","messageId":"xmqqedbjtqnr.fsf@gitster.g","threadId":"61257","inReplyTo":"20240405191714.GA2561807@coredump.intra.peff.net","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-05T22:01:44Z","receivedAt":"2024-04-05T22:01:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, this seems pretty reasonable. I think we've traditionally been\n> hesitant to pass or return structs by value, but that's mostly\n> superstition.\n\nWe should still be hesitant against the practice to the same degree\nthat we are hesitant against struct assignment, especially when the\nstruct is of nontrivial size, or the struct has a pointer member\nwhose memory ownership semantics goes against shallow copying of the\nstruct.\n\nIn this particular case, I do not know offhand if .strftime_fmt is\nsafe to be shallowly copied, but I trust you two know and/or have\nalready looked at the implications.\n\nThanks.\n"},{"id":"492374","messageId":"88f2063e-bddb-4479-a0c1-5bad582b9fe9@web.de","threadId":"61257","inReplyTo":"xmqqedbjtqnr.fsf@gitster.g","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-06T07:06:00Z","receivedAt":"2024-04-06T07:06:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 06.04.24 um 00:01 schrieb Junio C Hamano:\n> Jeff King <peff@peff.net> writes:\n>\n>> Yeah, this seems pretty reasonable. I think we've traditionally been\n>> hesitant to pass or return structs by value, but that's mostly\n>> superstition.\n>\n> We should still be hesitant against the practice to the same degree\n> that we are hesitant against struct assignment, especially when the\n> struct is of nontrivial size, or the struct has a pointer member\n> whose memory ownership semantics goes against shallow copying of the\n> struct.\n>\n> In this particular case, I do not know offhand if .strftime_fmt is\n> safe to be shallowly copied, but I trust you two know and/or have\n> already looked at the implications.\n\ndate_mode_from_type() sets .strftime_fmt to NULL in the struct date_mode\nit creates and returns.  Giving a reference to it to date_mode_release()\nis a safe NOOP.\n\nshow_date() passes .strftime_fmt to strbuf_addftime() and does not\nretain a copy or change it.\n\nshow_ident_date() only passes its struct date_mode parameter to\nshow_date().\n\nformat_person_part() only passes its struct date_mode parameter to\nshow_ident_date().\n\nformat_reflog_person() only passes its struct date_mode parameter to\nformat_person_part().\n\nget_reflog_selector() only passes its struct date_mode parameter to\nshow_date().\n\nshow_reflog_message() only passes its struct date_mode parameter to\nget_reflog_selector().\n\nThe patch doesn't change any other function signatures.\n\nstrbuf_addftime() scans the format string and passes it to\nstrbuf_expand_step(), skip_prefix() and strftime(3).  None of them\nchange it or retain a copy of the pointer.\n\nIn other words: This patch mostly affects the read-only side of struct\ndate_mode handling, and the date_mode_from_type() part is benign as\nwell.\n\nRené\n"},{"id":"492424","messageId":"20240407012844.GC1085004@coredump.intra.peff.net","threadId":"61257","inReplyTo":"xmqqedbjtqnr.fsf@gitster.g","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-07T01:28:44Z","receivedAt":"2024-04-07T01:28:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 03:01:44PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Yeah, this seems pretty reasonable. I think we've traditionally been\n> > hesitant to pass or return structs by value, but that's mostly\n> > superstition.\n> \n> We should still be hesitant against the practice to the same degree\n> that we are hesitant against struct assignment, especially when the\n> struct is of nontrivial size, or the struct has a pointer member\n> whose memory ownership semantics goes against shallow copying of the\n> struct.\n\nGood point. There are really two thresholds: is this something that\nshould be totally forbidden, and is this something that is generally a\ngood idea. I think the answers here are \"no\" and \"yes\" respectively.\n\nIt is an OK solution for \"plain old data\" types like date_mode that are\nessentially just marshalling arguments, but not for more object-oriented\ncode that might have ownership over pointers.\n\n> In this particular case, I do not know offhand if .strftime_fmt is\n> safe to be shallowly copied, but I trust you two know and/or have\n> already looked at the implications.\n\nRené already went through each caller, but yeah, I think it is fine\nhere. This whole thing is just a convenience over having callers pass\naround a separated (enum, strftime_fmt, local) triple.\n\n-Peff\n"},{"id":"492563","messageId":"xmqqo7ajdc6y.fsf@gitster.g","threadId":"61257","inReplyTo":"20240407012844.GC1085004@coredump.intra.peff.net","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T16:57:41Z","receivedAt":"2024-04-08T16:57:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> We should still be hesitant against the practice to the same degree\n>> that we are hesitant against struct assignment, especially when the\n>> struct is of nontrivial size, or the struct has a pointer member\n>> whose memory ownership semantics goes against shallow copying of the\n>> struct.\n>\n> Good point. There are really two thresholds: is this something that\n> should be totally forbidden, and is this something that is generally a\n> good idea. I think the answers here are \"no\" and \"yes\" respectively.\n\nI agree with your conclusion but I found the above a bit confusing.\n\nBetween \"totally horrible, do not even think about it\" (0%) and\n\"that is of course an excellent idea\" (100%), you want to have two\npoints \"might have some merit but not acceptable\" (33%) and\nsomething else that is less than \"of course an excellent idea\" but\nstill acceptable (66%)?  I would not phrase the last threshold \"is\ngenerally a good idea\", though.  \"It is not generally a good idea,\nbut in this case, it is an adequate solution\", maybe.\n\n"},{"id":"492564","messageId":"20240408170938.GA1629595@coredump.intra.peff.net","threadId":"61257","inReplyTo":"xmqqo7ajdc6y.fsf@gitster.g","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-08T17:09:38Z","receivedAt":"2024-04-08T17:09:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 08, 2024 at 09:57:41AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> We should still be hesitant against the practice to the same degree\n> >> that we are hesitant against struct assignment, especially when the\n> >> struct is of nontrivial size, or the struct has a pointer member\n> >> whose memory ownership semantics goes against shallow copying of the\n> >> struct.\n> >\n> > Good point. There are really two thresholds: is this something that\n> > should be totally forbidden, and is this something that is generally a\n> > good idea. I think the answers here are \"no\" and \"yes\" respectively.\n> \n> I agree with your conclusion but I found the above a bit confusing.\n> \n> Between \"totally horrible, do not even think about it\" (0%) and\n> \"that is of course an excellent idea\" (100%), you want to have two\n> points \"might have some merit but not acceptable\" (33%) and\n> something else that is less than \"of course an excellent idea\" but\n> still acceptable (66%)?  I would not phrase the last threshold \"is\n> generally a good idea\", though.  \"It is not generally a good idea,\n> but in this case, it is an adequate solution\", maybe.\n\nSorry, yes, I flipped the boolean on the second one. I should have said\n\"is it forbidden\" and \"is it generally a bad idea\", in which case the\nanswers are \"no\" and \"yes\".\n\nOr if you prefer, as I stated it, the answers to both are \"no\". ;)\n\nI.e., it is OK to use here but it is not something we should be doing\nall the time. Sorry for the confusion.\n\n-Peff\n"},{"id":"492805","messageId":"ZhhU7YBY6brbyZDm@google.com","threadId":"61257","inReplyTo":"520da361-1b80-4ba3-87b2-86d6fdfc18b5@web.de","subject":"Re: [RFC][PATCH] t-prio-queue: simplify using compound literals","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-04-11T21:23:57Z","receivedAt":"2024-04-11T21:24:02Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.04.02 20:30, René Scharfe wrote:\n> Test names like \"basic\" are mentioned seven times in the code (ignoring\n> case): Twice when defining the input and result macros, thrice when\n> defining the test function, and twice again when calling it.  Reduce\n> that to a single time by using compound literals to pass the input and\n> result arrays via TEST_INPUT to test_prio_queue().\n> \n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> C99 added compound literals.  Are we ready to use them?\n> \n> Test definitions become more compact, but look busier due to the added\n> punctuation.  We could hide some of it with a sugary macro like this:\n> #define INT_ARRAY(...) ((int []){ __VA_ARGS__ })\n\nI definitely like this approach. Reading the original test involved a\nlot of jumping around to see which TEST() checked which set of\narguments, now they're all right there in the TEST() call.\n\nAgreed that an INT_ARRAY macro or similar would make it look a bit\nnicer, but even without that I think this is a good improvement.\n\nI am in favor of expanding our C99 usage, and ISTM that a unit test is a\nfriendlier test balloon than production code (although perhaps one that\nis easier to miss).\n"}]}