[PATCH v5 0/1] MR9362: opengl32: Simplify extension tokenization.
This was requested in !9263 review, but ignored for some reason. -- v5: opengl32: Simplify extension tokenization. https://gitlab.winehq.org/wine/wine/-/merge_requests/9362
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/opengl32/unix_wgl.c | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/dlls/opengl32/unix_wgl.c b/dlls/opengl32/unix_wgl.c index 4f346b39226..ea3119195b8 100644 --- a/dlls/opengl32/unix_wgl.c +++ b/dlls/opengl32/unix_wgl.c @@ -1233,29 +1233,22 @@ static void make_context_current( TEB *teb, const struct opengl_funcs *funcs, HD } else { - const char *str = (const char *)funcs->p_glGetString( GL_EXTENSIONS ); - size_t len = strlen( str ); - const char *p; - char *ext; - if (!str) str = ""; - if ((len = strlen( str )) && str[len - 1] == ' ') len--; - if (*str) size++; - for (p = str; p < str + len; p++) if (*p == ' ') size++; - if (!(extensions = malloc( size * sizeof(*extensions) + len + 1 ))) return; - ext = (char *)&extensions[size]; - memcpy( ext, str, len ); - ext[len] = 0; - if (*ext) extensions[count++] = ext; - while (*ext) + const char *str, *ptr; + char *out; + + if (!(str = (const char *)funcs->p_glGetString( GL_EXTENSIONS ))) str = ""; + for (ptr = str; *ptr; ptr++) if (ptr == str || ptr[-1] == ' ') size++; + + if (!(extensions = malloc( size * sizeof(*extensions) + (ptr - str + 1) ))) return; + + for (out = (char *)&extensions[size], ptr = str; *ptr; out++, ptr++) { - if (*ext == ' ') - { - *ext = 0; - extensions[count++] = ext + 1; - } - ext++; + if (ptr == str || ptr[-1] == ' ') extensions[count++] = out; + *out = *ptr == ' ' ? 0 : *ptr; } - assert( count + ARRAYSIZE(legacy_extensions) - 1 == size ); + if (ptr != str) *out = 0; + + assert( count + ARRAY_SIZE(legacy_extensions) - 1 == size ); } if (!disabled && !(disabled = query_opengl_option( "DisabledExtensions" ))) disabled = ""; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9362
I'm sorry but this has been merged without my approval, when I had explicitly requested changes to the code which I found ugly and inconsistent, by squeezing the change to a different MR which I wasn't assigned to.
I really don’t see why this is being made into such a big issue. The final version of !9263 never received any comments. I tried to address all of your comments, both ones that I found reasonable and the ones that I didn't. I realize now that you’re probably referring to a comment buried in a thread where I was focused on explaining how those basic OpenGL functions work. If there was a problem, one more simple comment would have been enough in my view. It was just an oversight. The whole change has always been part of !9032 in my tree. I split it out because that part was ready sooner, but I think applying it all together seemed equally fine. Since I considered !9032 ready, I saw no reason to keep it as a draft. I also believe merging it was the right call. Nothing in this MR seems worth delaying a release cycle of testing for a major feature. I can’t help but notice that not a single comment in !9263 was about correctness. I also still think that if you’d considered my comments about the longer-term plans, it would be clearer why the initial version was a better fit than this ad-hoc tokenizing approach.
I believe code should be kept consistent to avoid wasting our time on such considerations.
Since we both agree it’s a waste of time, could you please stop making such a big deal out of it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9362#note_121165
I realize now that you’re probably referring to a comment buried in a thread where I was focused on explaining how those basic OpenGL functions work. If there was a problem, one more simple comment would have been enough in my view. It was just an oversight.
It was not *burried* in a thread, I was the third comment of a thread which I explicitly left open after closing all of the others. In that comment, I said "I'd prefer to make the code readable as suggested still" which seems very explicit to me. Now, I wasn't and still not really complaining about `ARRAYSIZE`, although it should simply never have been introduced. I was and still am complaining about the horrible tokenization logic, which I cannot understand how it felt acceptable to you in the first place, when you're get picky about other details. I wasn't making such a big deal out of it but now that you're nitpicking on other details I'm starting to get annoyed, yes. That code: 1) calls `strlen` on a pointer which is *later* checked not to be NULL, UB which will either crash or allow the compiler to elide the NULL check entirely. 2) Unnecessarily calls `strlen` *again* after the NULL check. 3) Iterates over the entire string a third time just to count spaces. 4) Iterates over the string *again* to copy it after allocation, and 5) Iterates over the string a *fifth* time to tokenize it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9362#note_121167
On Fri Nov 7 14:21:58 2025 +0000, Rémi Bernon wrote:
I realize now that you’re probably referring to a comment buried in a thread where I was focused on explaining how those basic OpenGL functions work. If there was a problem, one more simple comment would have been enough in my view. It was just an oversight. It was not *burried* in a thread, it was the third comment of a thread which I explicitly left open after closing all of the others. In that comment, I said "I'd prefer to make the code readable as suggested still" which seems very explicit to me. That comment was never addressed, or replied to, and there was no reason for me to request anything more. Now, I wasn't and still not really complaining about `ARRAYSIZE`, although it should simply never have been introduced. I was and still am complaining about the horrible tokenization logic, which I cannot understand how it felt acceptable to you in the first place, when you get picky about other details. I wasn't making such a big deal out of it but now that you're nitpicking on other details I'm starting to get annoyed, yes. That code: 1) calls `strlen` on a pointer which is *later* checked not to be NULL, UB which will either crash or allow the compiler to elide the NULL check entirely. 2) Unnecessarily calls `strlen` *again* after the NULL check. 3) Iterates over the entire string a third time just to count spaces. 4) Iterates over the string *again* to copy it after allocation, and 5) Iterates over the string a *fifth* time to tokenize it. I don't know what nitpicking you mean. I made a single request to remove an unrelated change; every other comment in this MR was strictly about correctness, not subjective nitpicking. I'm genuinely curious which of the bugs I pointed out would you rather have committed?
The extra `strlen` should obviously be removed, but the rest of your comment presented highly subjective opinions as objective truth. The disrespectful tone doesn’t help either. Multiple iterations aren’t wrong on their own. Moving more logic into a for statement reduces line count, but not complexity. It’s nice when a special case can be removed by generalizing a loop, but when you structure it like `for (i = 0; ... ) { if (i == 0) {...} }`, that defeats the point, in my humble opinion. It also makes the code less intuitive and thus more error-prone (as shown in earlier revisions of this MR). That said, I don’t mind the change. Addressing the remaining comment would still be a polite thing to do. Ultimately, I think both variants are ugly, the underlying issue is that it’s inconvenient to use strings to represent extensions. I have a draft that tokenizes them into enum values. With that in place, and a `parse_extension` helper that looks up all known extensions and returns an enum id, the whole code could be just: ``` for (iter = str; iter; iter = strchr( iter, ' ' )) { if (*iter == ' ') iter++; if ((extensions[count] = parse_extension( iter ))) count++; } ``` That requires several side quests first, which is why I sent the intermediate version (which is already a clear improvement). Ultimately, it’s also very convenient for `parse_extension` to accept space-terminated input. I implemented that in the initial variant of !9263, but you had a vague, unspecified issue with it and didn’t seem interested in my longer-term plans. I reworked that part back then, but it will come back, so I’d be interested to know what was so special about the open-coded `strcmp` that it was worth requesting a change to the entire series (while open-coding something like `strtok` was apparently fine as I think it should). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9362#note_121382
On Mon Nov 10 18:00:04 2025 +0000, Jacek Caban wrote:
I don't know what nitpicking you mean. I made a single request to remove an unrelated change; every other comment in this MR was strictly about correctness, not subjective nitpicking. I'm genuinely curious which of the bugs I pointed out would you rather have committed? The extra `strlen` should obviously be removed, but the rest of your comment presented highly subjective opinions as objective truth. The disrespectful tone doesn’t help either. Multiple iterations aren’t wrong on their own. Moving more logic into a for statement reduces line count, but not complexity. It’s nice when a special case can be removed by generalizing a loop, but when you structure it like `for (i = 0; ... ) { if (i == 0) {...} }`, that defeats the point, in my humble opinion. It also makes the code less intuitive and thus more error-prone (as shown in earlier revisions of this MR). That said, I don’t mind the change. Addressing the remaining comment would still be a polite thing to do. Ultimately, I think both variants are ugly, the underlying issue is that it’s inconvenient to use strings to represent extensions. I have a draft that tokenizes them into enum values. With that in place, and a `parse_extension` helper that looks up all known extensions and returns an enum id, the whole code could be just: ``` for (iter = str; iter; iter = strchr( iter, ' ' )) { if (*iter == ' ') iter++; if ((extensions[count] = parse_extension( iter ))) count++; } ``` That requires several side quests first, which is why I sent the intermediate version (which is already a clear improvement). Ultimately, it’s also very convenient for `parse_extension` to accept space-terminated input. I implemented that in the initial variant of !9263, but you had a vague, unspecified issue with it and didn’t seem interested in my longer-term plans. I reworked that part back then, but it will come back, so I’d be interested to know what was so special about the open-coded `strcmp` that it was worth requesting a change to the entire series (while open-coding something like `strtok` was apparently fine as I think it should). I'm sorry if my tone is disrespectful, then I think I could argue that I found it disrespectful to ignore my comment in the first place, and then to reverse the situation by asking for justification about a change that I originally requested.
Sure, you can always say that most of what I say is a "highly subjective opinion", but if that's the case it's IMO going to be true for almost everything we write. Having preferences, including about style, and requesting changes to make the code match them is perfectly acceptable and expected as far as I know. Then I would argue that these are not so highly subjective opinions, but rather that the number of iterations here probably matters as these strings usually get very large. A more subjective opinion here would be that the code was barely readable with no spacing, still IMO perfectly acceptable request to be made and which would have to be addressed before merging. Anyway, regarding the longer term plans that you said I wasn't interested in: I was not aware that you had any, and you never said or linked anything about them. I reviewed one MR, looking at the changes that were in that MR, and made my comments based on these only. If you had more changes planned, that would have explained why things were like they were, I think it was up to you to link them there. I'm not and wasn't strongly attached to my suggestion, it was done quickly (and bogus as you said, and thank you for uncovering my mistakes) and I expected you to take over it, or reply with some arguments why your code was better. If you have something better planned, I will be very happy about it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9362#note_121394
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9362
participants (3)
-
Jacek Caban (＠jacek) -
Rémi Bernon
-
Rémi Bernon (＠rbernon)