{"thread":{"id":"63495","subject":"[Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning","startedAt":"2025-05-21T19:14:12Z","lastAt":"2025-05-23T03:26:03Z","messageCount":3,"participants":["Octavio Carneiro","Junio C Hamano","O. C."],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"518606","messageId":"20250521191352.30849-1-ocarneiro1@gmail.com","threadId":"63495","inReplyTo":null,"subject":"[Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning","fromName":"Octavio Carneiro","fromEmail":"ocarneiro1@gmail.com","sentAt":"2025-05-21T19:13:52Z","receivedAt":"2025-05-21T19:14:12Z","isPatch":true,"sender":{"key":"ocarneiro1@gmail.com","avatar":null},"body":"A comparison between graph_line->width (of type size_t) and git_graph->width (of type int) causes -Wsign-compare to complain.\n\nLooking at the git_graph struct definition, its size variables are int-typed.\n\nTherefore, I changed the type of graph_line->width to also be a int, thus removing the warning trigger.\n\nSigned-off-by: Octavio Carneiro \n---\n graph.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 26f6fbf000..cb2221700e 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -1,5 +1,3 @@\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n-\n #include \"git-compat-util.h\"\n #include \"gettext.h\"\n #include \"config.h\"\n@@ -115,7 +113,7 @@ static const char *column_get_color_code(unsigned short color)\n \n struct graph_line {\n \tstruct strbuf *buf;\n-\tsize_t width;\n+\tint width;\n };\n \n static inline void graph_line_addch(struct graph_line *line, int c)\n-- \n2.34.1\n\n"},{"id":"518628","messageId":"xmqqtt5d7e9z.fsf@gitster.g","threadId":"63495","inReplyTo":"20250521191352.30849-1-ocarneiro1@gmail.com","subject":"Re: [Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T23:08:40Z","receivedAt":"2025-05-21T23:08:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Octavio Carneiro writes:\n\n> A comparison between graph_line->width (of type size_t) and git_graph->width (of type int) causes -Wsign-compare to complain.\n>\n> Looking at the git_graph struct definition, its size variables are int-typed.\n>\n> Therefore, I changed the type of graph_line->width to also be a int, thus removing the warning trigger.\n\nAn obvious question after reading the above explanation is if it\nalso would be a valid solution to change the type of git_graph.width\nto match the type of graph_line.width instead. And if so, what was\nthe reason why you chose to match their types in this direction?\n\n Side note: I personally prefer, when not dealing with things\n whose size MUST be expressed with size_t, to use platform\n natural \"int\" to count them, and on-display width of \"git log\n --graph\" output is certainly something that should not exceed\n thousands for sane people, so I am OK with using \"int\" myself,\n but if there are obvious two choices and a commit chose one, we\n want to see the reasoning behind the choice explained in the\n proposed commit log message.\n\nBy the way, these lines are overly long. We aim to limit our (physical) line\nlength to around 70 chars or so by line wrapping.\n\n> diff --git a/graph.c b/graph.c\n> index 26f6fbf000..cb2221700e 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -1,5 +1,3 @@\n> -#define DISABLE_SIGN_COMPARE_WARNINGS\n> -\n> #include \"git-compat-util.h\"\n> #include \"gettext.h\"\n> #include \"config.h\"\n> @@ -115,7 +113,7 @@ static const char *column_get_color_code(unsigned short color)\n> \n> struct graph_line {\n> \tstruct strbuf *buf;\n> -\tsize_t width;\n> +\tint width;\n> };\n\nWhen functions like graph_line_addstr() widens the line width, it\ndoes things like\n\n\tline->width += strlen(s);\n\nwhich could make line->width eventually overflow no matter whether\nits type is \"int\" or \"size_t\". Making .width that used to be\n\"size_t\" into \"int\" may make it easier (simply because \"int\" is\noften much narrower than \"size_t\") to happen, but your change does\nnot make the code worse than the original.\n\nSince I anticipate some senior developers advocate for using\n\"size_t\" uniformly (instead of the platform natural \"int\"), I'll let\nthem speak up before touching this patch.\n\nThanks.\n\n\nP.S. \n\nA totally unrelated tangent.\n\nI notice that graph->width and line->width code assumes that a\nsingle byte is always one display column wide. It might be an\nintersting GSoC or Outreachy project to use Unicode graphics instead\nof limiting us to ASCII art to draw these graph.\n\n"},{"id":"518744","messageId":"CADHqMhY85ZUd=NVq9dfUrBZEzEhfqwc-iYh=gcQ9J1hg=EkAsg@mail.gmail.com","threadId":"63495","inReplyTo":"xmqqtt5d7e9z.fsf@gitster.g","subject":"Re: [Newcomer][PATCH] graph.c: change graph_line->width type to int to remove sign-compare warning","fromName":"O. C.","fromEmail":"ocarneiro1@gmail.com","sentAt":"2025-05-23T03:25:50Z","receivedAt":"2025-05-23T03:26:03Z","isPatch":true,"sender":{"key":"ocarneiro1@gmail.com","avatar":null},"body":"On Wed, May 21, 2025 at 8:08 PM Junio C Hamano wrote:\n> An obvious question after reading the above explanation is if it\n> also would be a valid solution to change the type of git_graph.width\n> to match the type of graph_line.width instead. And if so, what was\n> the reason why you chose to match their types in this direction?\n\nWhile writing the patch, I saw how the git_graph and graph_line structs\nrelated to each other, and understood git_graph as being the \"more\nimportant\" struct in this context. So, it seemed more reasonable to\nhave graph_line.width comply with git_graph.width type.\n\nI do see the opposite point, taking notice of graph_line.width interactions\nwith \"size_t\". With those in mind, having the type as \"size_t\" does\nalso make a lot of sense.\n\nHowever, considering that:\n> \"git log --graph\" output is certainly something that should not exceed\n> thousands for sane people\nIt feels intuitive to \" use platform natural \"int\" \" as you put it.\n\n> By the way, these lines are overly long. We aim to limit our (physical) line\n> length to around 70 chars or so by line wrapping.\nSorry about that, I'll keep them shorter.\n\nLastly, thanks for the quick and careful response to the patch. As a\nfirst timer\nI appreciate the discussion.\n\n- Octavio\n"}]}