Re: [Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning
Octavio Carneiro <ocarneiro1@gmail.com> writes:
Show 5 quoted lines
> A comparison between graph_line->width (of type size_t) and git_graph->width (of type int) causes -Wsign-compare to complain.
>
> Looking at the git_graph struct definition, its size variables are int-typed.
>
> Therefore, I changed the type of graph_line->width to also be a int, thus removing the warning trigger.
An obvious question after reading the above explanation is if it also would be a valid solution to change the type of git_graph.width to match the type of graph_line.width instead. And if so, what was the reason why you chose to match their types in this direction?
Side note: I personally prefer, when not dealing with things
whose size MUST be expressed with size_t, to use platform
natural "int" to count them, and on-display width of "git log
--graph" output is certainly something that should not exceed
thousands for sane people, so I am OK with using "int" myself,
but if there are obvious two choices and a commit chose one, we
want to see the reasoning behind the choice explained in the
proposed commit log message.By the way, these lines are overly long. We aim to limit our (physical) line length to around 70 chars or so by line wrapping.
Show 17 quoted lines
> diff --git a/graph.c b/graph.c
> index 26f6fbf000..cb2221700e 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -1,5 +1,3 @@
> -#define DISABLE_SIGN_COMPARE_WARNINGS
> -
> #include "git-compat-util.h"
> #include "gettext.h"
> #include "config.h"
> @@ -115,7 +113,7 @@ static const char *column_get_color_code(unsigned short color)
>
> struct graph_line {
> struct strbuf *buf;
> - size_t width;
> + int width;
> };When functions like graph_line_addstr() widens the line width, it does things like
line->width += strlen(s);
which could make line->width eventually overflow no matter whether its type is "int" or "size_t". Making .width that used to be "size_t" into "int" may make it easier (simply because "int" is often much narrower than "size_t") to happen, but your change does not make the code worse than the original.
Since I anticipate some senior developers advocate for using "size_t" uniformly (instead of the platform natural "int"), I'll let them speak up before touching this patch.
Thanks.
P.S.
A totally unrelated tangent.
I notice that graph->width and line->width code assumes that a single byte is always one display column wide. It might be an intersting GSoC or Outreachy project to use Unicode graphics instead of limiting us to ASCII art to draw these graph.
