(PLease, avoid top-posting on this list) On Fri, Dec 22, 2023 at 11:57:25PM +0530, Chandra Pratap wrote: > Thanks for the feedback, Torsten. I was working on improving the rest of > sideband.c as you suggested when I encountered this snippet on line 82: > > while (0 < n && isspace(*src)) { > strbuf_addch(dest, *src); > src++; > n--; > } > > Here, we are decreasing the value of an unsigned type to potentially below > 0 which may lead to overflow and result in some nasty bug. In this case, > is it wise to continue with replacing 'int n' with 'size_t n' as the > NEEDSWORK tag suggests or should we improve upon the rest of the file > and revert 'size_t n' to 'int n' ? Yes, that could be a solution. But. In general, we are are striving to use size_t for all objects that live in memory, and that is a long term thing. Careful review is needed, and that is what you just did. If we look at this code again: while (0 < n && isspace(*src)) { strbuf_addch(dest, *src); src++; n--; } When n is signed, it makes sense to use "0 < n". However, if I think about it, it should work for unsigned as well, wouldn't it ? We can leave it as is, or replace it by while (n && isspace(*src)) { strbuf_addch(dest, *src); src++; n--; } > > On Fri, 22 Dec 2023 at 23:27, Torsten Bögershausen <tboegi@xxxxxx> wrote: > > > > On Fri, Dec 22, 2023 at 05:01:09PM +0000, Chandra Pratap via GitGitGadget wrote: > > > From: Chandra Pratap <chandrapratap3519@xxxxxxxxx> > > > > > > Replace int with size_t for clarity and remove the > > > 'NEEDSWORK' tag associated with it. > > > > > > Signed-off-by: Chandra Pratap <chandrapratap3519@xxxxxxxxx> > > > --- > > > sideband.c: replace int with size_t for clarity > > > > > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1625%2FChand-ra%2Fdusra-v1 > > > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1625/Chand-ra/dusra-v1 > > > Pull-Request: https://github.com/gitgitgadget/git/pull/1625 > > > > > > sideband.c | 3 +-- > > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > > > diff --git a/sideband.c b/sideband.c > > > index 6cbfd391c47..1599e408d1b 100644 > > > --- a/sideband.c > > > +++ b/sideband.c > > > @@ -69,9 +69,8 @@ void list_config_color_sideband_slots(struct string_list *list, const char *pref > > > * of the line. This should be called for a single line only, which is > > > * passed as the first N characters of the SRC array. > > > * > > > - * NEEDSWORK: use "size_t n" instead for clarity. > > > */ > > > -static void maybe_colorize_sideband(struct strbuf *dest, const char *src, int n) > > > +static void maybe_colorize_sideband(struct strbuf *dest, const char *src, size_t n) > > > { > > > int i; > > > > > > > Thanks for working on this. > > There is, however, more potential for improvements. > > First of all: the "int i" from above. > > Further down, we read > > for (i = 0; i < ARRAY_SIZE(keywords); i++) { > > > > However, a size of an array can never be negative, so that > > an unsigned data type is a better choice than a signed. > > And, arrays can have more elements than an int can address, > > at least in theory. > > For a reader it makes more sense, to replace > > int i; > > with > > size_t i; > > > > > > And further down, there is another place for improvments: > > > > int len = strlen(p->keyword); > > if (n < len) > > continue; > > > > Even here, a strlen is never negative. And a size_t is the choice for len, > > since all "modern" implementations declare strlen() to return size_t > > > > Do you think that you can have a look at these changes ? > > > > I will be happy to do a review, and possibly other people as well. >