From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Junio C Hamano <gitster@pobox.com>
Cc: Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison
Date: Fri, 19 Aug 2022 16:52:30 +0200 (CEST) [thread overview]
Message-ID: <nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr> (raw)
In-Reply-To: <xmqqr11gxm04.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 5547 bytes --]
Hi Junio,
On Tue, 16 Aug 2022, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > The scope of this patch series is to fix running the tests in Visual
> > Studio when building using CMake.
>
> That's only the perspective of who cares about VS+CMake. From my
> point of view who wants a healthy Git overall, the priority is
> different. "add -p" fix is wider than the context of VS, and I do
> not discount the need that we need to fix it before we can call
> VS+CMake issue fixed (and I do not mean to say it is unnecessary to
> fix VS+CMake). It's just this one can proceed with help by those
> who do not care about or have no environment to help with VS+CMake
> because it is more generic, and I do not think you mind to make the
> rest of the narrower VS+CMake topic _depend_ on it.
Sure.
But if I pull out that patch into its code contribution, all of a sudden
the scope of _that_ contribution is to address an implicit signed/unsigned
cast. What other purpose could it have? And if we're addressing that
signed/unsigned cast, we cannot just fix this single one. We need to fix
them all, no?
So let's have a look at that project, since you are implicitly
volunteering me for it. We do have this in our `config.mak.dev`:
# These are disabled because we have these all over the place.
DEVELOPER_CFLAGS += -Wno-empty-body
DEVELOPER_CFLAGS += -Wno-missing-field-initializers
DEVELOPER_CFLAGS += -Wno-sign-compare
DEVELOPER_CFLAGS += -Wno-unused-parameter
endif
Note the `-Wno-sign-compare` part. If I comment that out, I get these
reports:
-- snip --
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
diff.h:210:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Werror=sign-compare]
add-interactive.c:207:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:210:17: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:238:16: error: comparison of integer expressions of different signedness: ‘int’ and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-patch.c:423:16: error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
add-interactive.c:379:25: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
add-interactive.c:389:11: error: comparison of integer expressions of different signedness: ‘ssize_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Werror=sign-compare]
[... 1260 more issues ...]
-- snap --
You see how that increases the amount of work you are asking me to do?
The worst part is that gcc (at least of version 9.4.0 which I have
available in my Ubuntu) does not even catch the line that is fixed by the
patch we are trying to discuss here. It catches 10 issues in
`add-patch.c`, but not the `i < file_diff->mode_change` one.
Neither does Visual C 2022, nor the gcc I have in Git for Windows' SDK,
which is v12.1.0.
So I would first need to find a tool that identifies all code locations
that compare signed with unsigned values.
And that's even before analyzing carefully how to address them (not all
instances will be as easy as upcasting from an unsigned bit to `int`, some
of those instances are about `size_t` vs `ssize_t`).
> > Pulling out this patch would break that patch series because it would
> > leave that breakage in place.
>
> We deal with topic that depends on another topic all the time, and I
> do not think there is anything that makes VS+CMake topic so special
> that it cannot be dependent on another topic. It's just the matter
> of splitting this out and make it [1/1], and make the remainder to
> [1-4/4] and mark them rely on add-p fix when you send the latter
> out, isn't it? Puzzled...
Sure, if you think that the signed/unsigned comparison project is more
important than fixing running Git's test suite inside Visual Studio, or
worse: if you are asking me to do a less than thorough job on those
signed/unsigned fixes by fixing only a single instance and leaving all
others unaddressed in a patch series that has nothing to do with Visual
Studio, then I understand how my stance could confuse you.
But my intention with this patch series is to fix running Git's test suite
in Visual Studio. And my intention is to provide a complete solution in my
patch series, no half measures.
As such, I would be loathe to have my authorship on a single patch that
solves neither the Visual Studio/CTest problem nor the vast majority of
the signed/unsigned problems. It would be incomplete in any way you look
at it. I would consider such a contribution lackluster, sloppy and
definitely not up to my standards.
Ciao,
Dscho
next prev parent reply other threads:[~2022-08-19 14:52 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-10 15:02 [PATCH 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 1/5] cmake: align CTest definition with Git's CI runs Johannes Schindelin via GitGitGadget
2022-08-10 17:48 ` Junio C Hamano
2022-08-16 10:11 ` Johannes Schindelin
2022-08-16 15:15 ` Junio C Hamano
2022-08-19 13:57 ` Johannes Schindelin
2022-08-11 11:18 ` Ævar Arnfjörð Bjarmason
2022-08-10 15:02 ` [PATCH 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows Johannes Schindelin via GitGitGadget
2022-08-11 11:22 ` Ævar Arnfjörð Bjarmason
2022-08-22 10:19 ` Johannes Schindelin
2022-08-23 7:34 ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-10 17:54 ` Junio C Hamano
2022-08-16 9:56 ` Johannes Schindelin
2022-08-16 15:10 ` Junio C Hamano
2022-08-19 14:52 ` Johannes Schindelin [this message]
2022-08-11 12:49 ` Phillip Wood
2022-08-16 10:00 ` Johannes Schindelin
2022-08-16 14:23 ` Phillip Wood
2022-08-19 14:07 ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 5/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-08-11 11:35 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:02 ` Johannes Schindelin
2022-08-11 12:58 ` Phillip Wood
2022-08-16 10:09 ` Johannes Schindelin
2022-08-16 14:27 ` Phillip Wood
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-23 8:30 ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-09-07 22:10 ` Victoria Dye
2022-10-18 14:02 ` Johannes Schindelin
2022-09-08 7:22 ` Ævar Arnfjörð Bjarmason
2022-09-28 6:55 ` Eric Sunshine
2022-08-23 8:31 ` [PATCH v2 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-09-08 7:39 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:03 ` Johannes Schindelin
2022-10-18 15:09 ` Ævar Arnfjörð Bjarmason
2022-09-08 23:37 ` Victoria Dye
2022-09-08 23:42 ` Victoria Dye
2022-09-08 23:58 ` Junio C Hamano
2022-10-18 14:03 ` Johannes Schindelin
2022-08-23 8:31 ` [PATCH v2 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
2022-09-08 7:34 ` Ævar Arnfjörð Bjarmason
2022-09-08 17:29 ` Victoria Dye
2022-09-08 3:51 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Victoria Dye
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-10-18 13:41 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-10-18 13:49 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-10-18 13:53 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-10-18 13:54 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:21 ` Johannes Schindelin
2022-10-18 10:59 ` [PATCH v3 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=nrr2312s-q256-61n7-2843-7r0s817rp432@tzk.qr \
--to=johannes.schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.