* [GSoC][PATCH 0/1] Use unsigned integral type for collection of bits.
@ 2024-02-24 11:26 Eugenio Gigante
2024-02-24 11:26 ` [GSoC][PATCH 1/1] add: use unsigned " Eugenio Gigante
0 siblings, 1 reply; 7+ messages in thread
From: Eugenio Gigante @ 2024-02-24 11:26 UTC (permalink / raw)
To: git; +Cc: sunshine, gitster, Eugenio Gigante
One of the suggested microprojects for the GSoC is to pick a field
of signed integral type that is used as as collection of bits, and
change its type to unsigned in case the code does not take advantage
of its MSb.
This patch finds an example in 'builtin/add.c'.
Eugenio Gigante (1):
add: use unsigned type for collection of bits
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
--
2.43.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
2024-02-24 11:26 [GSoC][PATCH 0/1] Use unsigned integral type for collection of bits Eugenio Gigante
@ 2024-02-24 11:26 ` Eugenio Gigante
2024-02-26 9:59 ` Christian Couder
0 siblings, 1 reply; 7+ messages in thread
From: Eugenio Gigante @ 2024-02-24 11:26 UTC (permalink / raw)
To: git; +Cc: sunshine, gitster, Eugenio Gigante
The function 'refresh' in 'builtin/add.c' declares 'flags' as signed,
while the function 'refresh_index' defined in 'read-cache-ll.h' expects an unsigned value.
Since in this case 'flags' represents a bag of bits, whose MSB is not used in special ways,
this commit changes the type of 'flags' to unsigned.
Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
---
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/add.c b/builtin/add.c
index ada7719561..393c10cbcf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
int i, ret = 0;
char *skip_worktree_seen = NULL;
struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
- int flags = REFRESH_IGNORE_SKIP_WORKTREE |
+ unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
(verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
seen = xcalloc(pathspec->nr, 1);
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
2024-02-24 11:26 ` [GSoC][PATCH 1/1] add: use unsigned " Eugenio Gigante
@ 2024-02-26 9:59 ` Christian Couder
2024-02-26 22:58 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Christian Couder @ 2024-02-26 9:59 UTC (permalink / raw)
To: Eugenio Gigante; +Cc: git, sunshine, gitster
On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante
<giganteeugenio2@gmail.com> wrote:
>
> The function 'refresh' in 'builtin/add.c' declares 'flags' as signed,
> while the function 'refresh_index' defined in 'read-cache-ll.h' expects an unsigned value.
It's not clear from the patch that refresh() passes 'flags' as an
argument to refresh_index(), so it might help reviewers a bit if you
could tell that.
> Since in this case 'flags' represents a bag of bits, whose MSB is not used in special ways,
> this commit changes the type of 'flags' to unsigned.
We prefer to use "let's change this and that" or just "change this and
that" rather than "this commit changes this and that", see
https://git-scm.com/docs/SubmittingPatches/#imperative-mood.
It might help if you could add a bit more explanation about why it's a
good thing to use an unsigned variable instead of a signed one. For
example you could say that it documents that we are not doing anything
funny with the MSB.
> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
> ---
> builtin/add.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
The patch looks correct, thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
2024-02-26 9:59 ` Christian Couder
@ 2024-02-26 22:58 ` Junio C Hamano
2024-02-29 19:44 ` [GSoC][PATCH v2 0/1] Use unsigned integral " Eugenio Gigante
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2024-02-26 22:58 UTC (permalink / raw)
To: Christian Couder; +Cc: Eugenio Gigante, git, sunshine
Christian Couder <christian.couder@gmail.com> writes:
> On Sat, Feb 24, 2024 at 12:28 PM Eugenio Gigante
> <giganteeugenio2@gmail.com> wrote:
>>
>> The function 'refresh' in 'builtin/add.c' declares 'flags' as
>> signed, while the function 'refresh_index' defined in
>> 'read-cache-ll.h' expects an unsigned value.
>
> It's not clear from the patch that refresh() passes 'flags' as an
> argument to refresh_index(), so it might help reviewers a bit if you
> could tell that.
Perhaps.
>> Since in this case 'flags' represents a bag of bits, whose MSB is
>> not used in special ways, this commit changes the type of 'flags'
>> to unsigned.
>
> We prefer to use "let's change this and that" or just "change this and
> that" rather than "this commit changes this and that", see
> https://git-scm.com/docs/SubmittingPatches/#imperative-mood.
Very true.
> It might help if you could add a bit more explanation about why it's a
> good thing to use an unsigned variable instead of a signed one. For
> example you could say that it documents that we are not doing anything
> funny with the MSB.
But doesn't the proposed log message already say so?
In any case, it would very much help to fold long lines and have a
blank line in between paragraphs to make the log message more
readable.
Thanks, both.
>
>> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
>> ---
>> builtin/add.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> The patch looks correct, thanks!
^ permalink raw reply [flat|nested] 7+ messages in thread
* [GSoC][PATCH v2 0/1] Use unsigned integral type for collection of bits.
2024-02-26 22:58 ` Junio C Hamano
@ 2024-02-29 19:44 ` Eugenio Gigante
2024-02-29 19:44 ` [PATCH v2 1/1] add: use unsigned " Eugenio Gigante
0 siblings, 1 reply; 7+ messages in thread
From: Eugenio Gigante @ 2024-02-29 19:44 UTC (permalink / raw)
To: gitster; +Cc: christian.couder, giganteeugenio2, git, sunshine
Improve the commit log message
based on reviewers feedback.
Eugenio Gigante (1):
add: use unsigned type for collection of bits
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
--
2.43.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/1] add: use unsigned type for collection of bits
2024-02-29 19:44 ` [GSoC][PATCH v2 0/1] Use unsigned integral " Eugenio Gigante
@ 2024-02-29 19:44 ` Eugenio Gigante
2024-02-29 19:52 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Eugenio Gigante @ 2024-02-29 19:44 UTC (permalink / raw)
To: gitster; +Cc: christian.couder, giganteeugenio2, git, sunshine
The 'refresh' function in 'builtin/add.c'
declares 'flags' as signed, and passes it
as an argument to the 'refresh_index' function,
which though expects an unsigned value.
Since in this case 'flags' represents
a bag of bits, whose MSB is not used
in special ways, change the type
of 'flags' to unsigned.
Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
---
builtin/add.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/add.c b/builtin/add.c
index ada7719561..393c10cbcf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
int i, ret = 0;
char *skip_worktree_seen = NULL;
struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
- int flags = REFRESH_IGNORE_SKIP_WORKTREE |
+ unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
(verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
seen = xcalloc(pathspec->nr, 1);
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/1] add: use unsigned type for collection of bits
2024-02-29 19:44 ` [PATCH v2 1/1] add: use unsigned " Eugenio Gigante
@ 2024-02-29 19:52 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2024-02-29 19:52 UTC (permalink / raw)
To: Eugenio Gigante; +Cc: christian.couder, git, sunshine
Eugenio Gigante <giganteeugenio2@gmail.com> writes:
> The 'refresh' function in 'builtin/add.c'
> declares 'flags' as signed, and passes it
> as an argument to the 'refresh_index' function,
> which though expects an unsigned value.
OK. This has become much easier to read by dropping the mention of
the header file where the callee happens to be declared.
Just FYI, in my private rewrite (that I'll discard), I made it like
so:
builtin/add.c:refresh() declares 'flags' as signed, and uses it
to call refresh_index() that expects an unsigned value.
> Since in this case 'flags' represents
> a bag of bits, whose MSB is not used
> in special ways, change the type
> of 'flags' to unsigned.
Good, too.
Will queue.
> Signed-off-by: Eugenio Gigante <giganteeugenio2@gmail.com>
> ---
> builtin/add.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index ada7719561..393c10cbcf 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -115,7 +115,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
> int i, ret = 0;
> char *skip_worktree_seen = NULL;
> struct string_list only_match_skip_worktree = STRING_LIST_INIT_NODUP;
> - int flags = REFRESH_IGNORE_SKIP_WORKTREE |
> + unsigned int flags = REFRESH_IGNORE_SKIP_WORKTREE |
> (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);
>
> seen = xcalloc(pathspec->nr, 1);
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-02-29 19:52 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-24 11:26 [GSoC][PATCH 0/1] Use unsigned integral type for collection of bits Eugenio Gigante
2024-02-24 11:26 ` [GSoC][PATCH 1/1] add: use unsigned " Eugenio Gigante
2024-02-26 9:59 ` Christian Couder
2024-02-26 22:58 ` Junio C Hamano
2024-02-29 19:44 ` [GSoC][PATCH v2 0/1] Use unsigned integral " Eugenio Gigante
2024-02-29 19:44 ` [PATCH v2 1/1] add: use unsigned " Eugenio Gigante
2024-02-29 19:52 ` Junio C Hamano
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 NNTP newsgroup(s).