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.