threads / patch / 60983
Subject: [GSoC][PATCH 0/1] Use unsigned integral type for collection of bits.
Eugenio GiganteOne 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
Eugenio Gigante[GSoC][PATCH 1/1] add: use unsigned type for collection of bits
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
Christian CouderRe: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
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!
Junio C HamanoRe: [GSoC][PATCH 1/1] add: use unsigned type for collection of bits
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!
Eugenio Gigante[GSoC][PATCH v2 0/1] Use unsigned integral type for collection of bits.
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
Eugenio Gigante[PATCH v2 1/1] add: use unsigned type for collection of bits
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
Junio C HamanoRe: [PATCH v2 1/1] add: use unsigned type for collection of bits
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);