git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: git@vger.kernel.org
Subject: "#define precompose_argv(c,v) /* empty */" is evil
Date: Thu, 06 Aug 2020 16:47:34 -0700	[thread overview]
Message-ID: <xmqqy2mribft.fsf@gitster.c.googlers.com> (raw)

I had seen an interesting compilation breakage today.  A topic adds
many more uses of argv-array API so I resolved semantic conflict
patch until "make builtins/submodule-helper.o" passed.  I failed to
spot one remaining breakage until I saw

    https://travis-ci.org/github/git/git/jobs/715668996

The problematic line was

    precompose_argv(diff_args.argc, diff_args.argv);

where diff_args used to be an argv_array and is now a strvec.

Why didn't I catch this in my local test?

$ git grep -n -e precompose_argv -- \*.h
compat/precompose_utf8.h:31:void precompose_argv(int argc, const char **argv);
git-compat-util.h:256:#define precompose_argv(c,v)

The problematic part is this one used on all platforms other than macOS:

    /* used on Mac OS X */
    #ifdef PRECOMPOSE_UNICODE
    #include "compat/precompose_utf8.h"
    #else
    #define precompose_str(in,i_nfd2nfc)
    #define precompose_argv(c,v)
    #define probe_utf8_pathname_composition()
    #endif

I am wondering if it is a good idea to use something like

    static inline void precompose_argv(int argc, const char **argv)
    {
	; /* nothing */
    }

instead.  As long as the compiler is reasonable enough, this should
not result in any code change in the result, except that it would
still catch wrong arguments, even if these two parameters are unused
and optimized out.


             reply	other threads:[~2020-08-06 23:47 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-06 23:47 Junio C Hamano [this message]
2020-08-07  0:01 ` "#define precompose_argv(c,v) /* empty */" is evil brian m. carlson
2020-08-07  0:23   ` Junio C Hamano
2020-08-07  1:32     ` brian m. carlson
2020-08-07  4:03       ` Junio C Hamano
2020-08-07  3:27     ` Jeff King
2020-08-07  4:09       ` Junio C Hamano
2020-08-07  4:34         ` Jeff King
2020-08-07  6:42           ` Junio C Hamano
2020-08-07  7:56 ` Torsten Bögershausen

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=xmqqy2mribft.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    /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).