Re: [PATCH} suppress "misleading indentation" gcc 11 warning
On 18 Dec, Tim Düsterhus wrote: > I'm not sure I understand the question correctly, but I assume you mean > the highlighting due to me directly linking to the lines? Arf, didn't realize that... Thanks! ~Nico
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Nicolas, Am 18.12.20 um 02:13 schrieb Nicolas CARPi: >> But even then it stomps over stuff like `a_eq_b` in >> contrib/ip6range/ip6range.c: > Could you please enlighten me as to why the diff of this function is of a > different color from the rest? First time I see this. > I'm not sure I understand the question correctly, but I assume you mean the highlighting due to me directly linking to the lines? You can select a range of line numbers by clicking the first line, holding Shift and clicking the second line. The URL bar will automatically updated to link to the range. Best regards Tim Düsterhus
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Dear Tim, > It'll probably get a quite bit smaller if you'd add a configuration file > that configures the indentation to happen using tabs instead of spaces. You're correct. It was just a quick and dirty try with default config to see if things can still compile after running it, I have no intention on proposing a patch. :) > But even then it stomps over stuff like `a_eq_b` in > contrib/ip6range/ip6range.c: Could you please enlighten me as to why the diff of this function is of a different color from the rest? First time I see this. Cheers and thanks to all the contributors for this great piece of software! ~Nico
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Nicolas, Am 17.12.20 um 22:30 schrieb Nicolas CARPi: > For purely illustration purposes, this is the kind of diff that would be > generated if one were to clang-format all the .c files: > > https://github.com/deltablot/haproxy/commit/878ba41e4742b39034686d2b6feb3c561edeb72a > > Showing 195 changed files with 177,997 additions and 174,367 deletions. It'll probably get a quite bit smaller if you'd add a configuration file that configures the indentation to happen using tabs instead of spaces. But even then it stomps over stuff like `a_eq_b` in contrib/ip6range/ip6range.c: https://github.com/deltablot/haproxy/commit/878ba41e4742b39034686d2b6feb3c561edeb72a#diff-f99a0be674dc22ec08274e8ae4e68be49a16d905afdeb918d31779898ba08ba9L74-R84 It definitely does not help readability :-) Best regards Tim Düsterhus PS: Prefer replying inline instead of top-posting and cut the quotes to only keep a small context.
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Dear list, For purely illustration purposes, this is the kind of diff that would be generated if one were to clang-format all the .c files: https://github.com/deltablot/haproxy/commit/878ba41e4742b39034686d2b6feb3c561edeb72a Showing 195 changed files with 177,997 additions and 174,367 deletions. oO ! Have a nice evening all (or day depending on your TZ). ~Nicolas On 17 Dec, Nicolas CARPi wrote: > Hey Tim, > > Sorry I wasn't clear in my message, I should have written that the doc > needs to be updated only if you ever chose to add this formatting check. > But as you stated, it would be a lot of work and commit noise for little > benefit and I understand completely why you would not want to do that. > > It's the kind of thing that benefit fresh projects. > > Best regards, > ~Nicolas > > > On 17 Dec, Tim Düsterhus wrote: > > Nicolas, > > > > Am 17.12.20 um 15:53 schrieb Nicolas CARPi: > > > To fail or not to fail. That is the question! :) > > > > > > I'd add to that the contributor documentation should be updated to add > > > steps for having a clang-format (or any other formatter) as a git > > > pre-hook. > > > > As someone who actually contributes to the C source code of HAProxy I > > disagree. > > > > 1) The codebase is fairly old and already has formatting issues (Tabs > > and Spaces mixed within single files). Initially running the > > autoformatter will create merge / rebase conflicts all over the place. > > 2) A few source code locations are intentionally manually formatted like > > they are. Autoformatters tend to stomp over that, breaking this > > intentional formatting, decreasing readability. > > 3) Clearly the actual contributors are not unhappy with the current > > situation, otherwise they would already have introduced such an > > autoformatting. > > 4) It would require contributors to set up that autoformatter (in the > > correct version, with the correct config, etc.), creating work for a > > small benefit. > > > > Best regards > > Tim Düsterhus > > > > > I'm partial to failing the build (in CI) to avoid using further > > > electricity if the first step (syntax) fails, as another commit fixing > > > the syntax will likely come after. > > > > > > ~Nico > > > > > > > -- > ~Nico > -- ~Nico
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Hey Tim, Sorry I wasn't clear in my message, I should have written that the doc needs to be updated only if you ever chose to add this formatting check. But as you stated, it would be a lot of work and commit noise for little benefit and I understand completely why you would not want to do that. It's the kind of thing that benefit fresh projects. Best regards, ~Nicolas On 17 Dec, Tim Düsterhus wrote: > Nicolas, > > Am 17.12.20 um 15:53 schrieb Nicolas CARPi: > > To fail or not to fail. That is the question! :) > > > > I'd add to that the contributor documentation should be updated to add > > steps for having a clang-format (or any other formatter) as a git > > pre-hook. > > As someone who actually contributes to the C source code of HAProxy I > disagree. > > 1) The codebase is fairly old and already has formatting issues (Tabs > and Spaces mixed within single files). Initially running the > autoformatter will create merge / rebase conflicts all over the place. > 2) A few source code locations are intentionally manually formatted like > they are. Autoformatters tend to stomp over that, breaking this > intentional formatting, decreasing readability. > 3) Clearly the actual contributors are not unhappy with the current > situation, otherwise they would already have introduced such an > autoformatting. > 4) It would require contributors to set up that autoformatter (in the > correct version, with the correct config, etc.), creating work for a > small benefit. > > Best regards > Tim Düsterhus > > > I'm partial to failing the build (in CI) to avoid using further > > electricity if the first step (syntax) fails, as another commit fixing > > the syntax will likely come after. > > > > ~Nico > > > -- ~Nico
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Hi guys, On Thu, Dec 17, 2020 at 05:39:45PM +0100, Tim Düsterhus wrote: > Ilya, > > [Willy: Note for you below] > > Am 17.12.20 um 10:01 schrieb ???: > > well, I met some projects that are insane on formatting. > > it is funny each time, when discussing new PR "please fix your formatting" > > (weeks spent on that), after PR is merged something is broken because of > > lack of testing. > > This is not about arbitrary formatting (Tabs vs Spaces or the bracing > style), but this is about *actually* misleading indentation. > > Misleading indentation is something that actually caused security > relevant bugs in other projects in the past. 'goto fail' would be an > example. > > This warning is good, it found an actual issue. It must stay. Indeed, while I hate when compilers complain about style, this category of bugs can keep you awake very late at night! > > Willy: > > Can you please check line 236 in plock.h (__pl_r = 0;). It is > misleadingly intended. In fact I believe the line might not be > necessary, because `__pl_r` should already be 0 after exiting the > `while` loop. Agreed, it's definitely a leftover of some previous code and makes no sense. I'll deal with it, thanks to you both for this report! Willy
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
чт, 17 дек. 2020 г. в 21:39, Tim Düsterhus : > Ilya, > > [Willy: Note for you below] > > Am 17.12.20 um 10:01 schrieb Илья Шипицин: > > well, I met some projects that are insane on formatting. > > it is funny each time, when discussing new PR "please fix your > formatting" > > (weeks spent on that), after PR is merged something is broken because of > > lack of testing. > > This is not about arbitrary formatting (Tabs vs Spaces or the bracing > style), but this is about *actually* misleading indentation. > > Misleading indentation is something that actually caused security > relevant bugs in other projects in the past. 'goto fail' would be an > example. > ok, so please do not aply this patch. > > This warning is good, it found an actual issue. It must stay. > ook. anyway I run Fedora Rawhide builds in CI, I'll see difference after fix :) > > --- > > Willy: > > Can you please check line 236 in plock.h (__pl_r = 0;). It is > misleadingly intended. In fact I believe the line might not be > necessary, because `__pl_r` should already be 0 after exiting the > `while` loop. > > Please either remove that line of remove a single tab at the start of > the line. > > The issue was introduced in 7122ab31b195edb511fecf9c20904701970b195f > ( > https://github.com/haproxy/haproxy/commit/7122ab31b195edb511fecf9c20904701970b195f#diff-f751477437a0042b907116079d933159c78054cfbfa1d955b389bb81a380R228 > ) > > > I'd say it is friendly mood not to push people to comply formatting (who > > cares of indentation ?) > > I do when it's misleading. > > Best regards > Tim Düsterhus >
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Nicolas, Am 17.12.20 um 15:53 schrieb Nicolas CARPi: > To fail or not to fail. That is the question! :) > > I'd add to that the contributor documentation should be updated to add > steps for having a clang-format (or any other formatter) as a git > pre-hook. As someone who actually contributes to the C source code of HAProxy I disagree. 1) The codebase is fairly old and already has formatting issues (Tabs and Spaces mixed within single files). Initially running the autoformatter will create merge / rebase conflicts all over the place. 2) A few source code locations are intentionally manually formatted like they are. Autoformatters tend to stomp over that, breaking this intentional formatting, decreasing readability. 3) Clearly the actual contributors are not unhappy with the current situation, otherwise they would already have introduced such an autoformatting. 4) It would require contributors to set up that autoformatter (in the correct version, with the correct config, etc.), creating work for a small benefit. Best regards Tim Düsterhus > I'm partial to failing the build (in CI) to avoid using further > electricity if the first step (syntax) fails, as another commit fixing > the syntax will likely come after. > > ~Nico >
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Ilya, [Willy: Note for you below] Am 17.12.20 um 10:01 schrieb Илья Шипицин: > well, I met some projects that are insane on formatting. > it is funny each time, when discussing new PR "please fix your formatting" > (weeks spent on that), after PR is merged something is broken because of > lack of testing. This is not about arbitrary formatting (Tabs vs Spaces or the bracing style), but this is about *actually* misleading indentation. Misleading indentation is something that actually caused security relevant bugs in other projects in the past. 'goto fail' would be an example. This warning is good, it found an actual issue. It must stay. --- Willy: Can you please check line 236 in plock.h (__pl_r = 0;). It is misleadingly intended. In fact I believe the line might not be necessary, because `__pl_r` should already be 0 after exiting the `while` loop. Please either remove that line of remove a single tab at the start of the line. The issue was introduced in 7122ab31b195edb511fecf9c20904701970b195f (https://github.com/haproxy/haproxy/commit/7122ab31b195edb511fecf9c20904701970b195f#diff-f751477437a0042b907116079d933159c78054cfbfa1d955b389bb81a380R228) > I'd say it is friendly mood not to push people to comply formatting (who > cares of indentation ?) I do when it's misleading. Best regards Tim Düsterhus
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
To fail or not to fail. That is the question! :) I'd add to that the contributor documentation should be updated to add steps for having a clang-format (or any other formatter) as a git pre-hook. I'm partial to failing the build (in CI) to avoid using further electricity if the first step (syntax) fails, as another commit fixing the syntax will likely come after. ~Nico On 17 Dec, Илья Шипицин wrote: > Well, there are formatters like clang format or similar. We can even run them > in pipeline. But it has nothing to do with compilation and we should not fail > build because of that > > On Thu, Dec 17, 2020, 5:35 PM Nicolas CARPi wrote: > > Dear list, > > Formatting on PRs is a hot topic, but IMHO it should be done by an > automated tool so we can forget about it and focus on the core of the > PR, not the style. A git pre-hook would be best. > > > I'd say it is friendly mood not to push people to comply formatting (who > cares > > of indentation ?) > I disagree with that view. No need to be as hardcore as go who won't > compile if there is an indentation issue, but still, I think it helps > readability and prevent bugs to have correct indentation (an incorrect > one might make you think that a block is part of a loop, when it is > not). > > Regards, > ~Nicolas CARPi > > PS: go ships with a formatting tool so this is a non issue in that > language > > > On 17 Dec, Илья Шипицин wrote: > > well, I met some projects that are insane on formatting. > > it is funny each time, when discussing new PR "please fix your > formatting" > > (weeks spent on that), after PR is merged something is broken because of > lack > > of testing. > > > > I'd say it is friendly mood not to push people to comply formatting (who > cares > > of indentation ?) > > > > чт, 17 дек. 2020 г. в 13:05, Tim Düsterhus <[1][1]t...@bastelstu.be>: > > > > Ilya, > > > > Am 17.12.20 um 08:26 schrieb Илья Шипицин: > > > gcc 11 (delivered with fedora rawhide) introduces indentation > check. > > > it is totally harmless. > > > > > > this patch suppresses it. > > > it resolves #998 > > > might be backported to all supported branches. > > > > > > > Can we please fix the actual issue instead of suppressing the > warning? > > In fact I believe that this line is incorrectly intended: > > > > [2][2]https://github.com/haproxy/haproxy/blob/ > > a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 > > > > Best regards > > Tim Düsterhus > > > > > > References: > > > > [1] mailto:[3]t...@bastelstu.be > > [2] [4]https://github.com/haproxy/haproxy/blob/ > a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 > > -- > ~Nico > > > References: > > [1] mailto:t...@bastelstu.be > [2] https://github.com/haproxy/haproxy/blob/ > [3] mailto:t...@bastelstu.be > [4] > https://github.com/haproxy/haproxy/blob/a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 -- ~Nico
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Well, there are formatters like clang format or similar. We can even run them in pipeline. But it has nothing to do with compilation and we should not fail build because of that On Thu, Dec 17, 2020, 5:35 PM Nicolas CARPi wrote: > Dear list, > > Formatting on PRs is a hot topic, but IMHO it should be done by an > automated tool so we can forget about it and focus on the core of the > PR, not the style. A git pre-hook would be best. > > > I'd say it is friendly mood not to push people to comply formatting (who > cares > > of indentation ?) > I disagree with that view. No need to be as hardcore as go who won't > compile if there is an indentation issue, but still, I think it helps > readability and prevent bugs to have correct indentation (an incorrect > one might make you think that a block is part of a loop, when it is > not). > > Regards, > ~Nicolas CARPi > > PS: go ships with a formatting tool so this is a non issue in that > language > > > On 17 Dec, Илья Шипицин wrote: > > well, I met some projects that are insane on formatting. > > it is funny each time, when discussing new PR "please fix your > formatting" > > (weeks spent on that), after PR is merged something is broken because of > lack > > of testing. > > > > I'd say it is friendly mood not to push people to comply formatting (who > cares > > of indentation ?) > > > > чт, 17 дек. 2020 г. в 13:05, Tim Düsterhus <[1]t...@bastelstu.be>: > > > > Ilya, > > > > Am 17.12.20 um 08:26 schrieb Илья Шипицин: > > > gcc 11 (delivered with fedora rawhide) introduces indentation > check. > > > it is totally harmless. > > > > > > this patch suppresses it. > > > it resolves #998 > > > might be backported to all supported branches. > > > > > > > Can we please fix the actual issue instead of suppressing the > warning? > > In fact I believe that this line is incorrectly intended: > > > > [2]https://github.com/haproxy/haproxy/blob/ > > a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 > > > > Best regards > > Tim Düsterhus > > > > > > References: > > > > [1] mailto:t...@bastelstu.be > > [2] > https://github.com/haproxy/haproxy/blob/a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 > > -- > ~Nico >
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Dear list, Formatting on PRs is a hot topic, but IMHO it should be done by an automated tool so we can forget about it and focus on the core of the PR, not the style. A git pre-hook would be best. > I'd say it is friendly mood not to push people to comply formatting (who cares > of indentation ?) I disagree with that view. No need to be as hardcore as go who won't compile if there is an indentation issue, but still, I think it helps readability and prevent bugs to have correct indentation (an incorrect one might make you think that a block is part of a loop, when it is not). Regards, ~Nicolas CARPi PS: go ships with a formatting tool so this is a non issue in that language On 17 Dec, Илья Шипицин wrote: > well, I met some projects that are insane on formatting. > it is funny each time, when discussing new PR "please fix your formatting" > (weeks spent on that), after PR is merged something is broken because of lack > of testing. > > I'd say it is friendly mood not to push people to comply formatting (who cares > of indentation ?) > > чт, 17 дек. 2020 г. в 13:05, Tim Düsterhus <[1]t...@bastelstu.be>: > > Ilya, > > Am 17.12.20 um 08:26 schrieb Илья Шипицин: > > gcc 11 (delivered with fedora rawhide) introduces indentation check. > > it is totally harmless. > > > > this patch suppresses it. > > it resolves #998 > > might be backported to all supported branches. > > > > Can we please fix the actual issue instead of suppressing the warning? > In fact I believe that this line is incorrectly intended: > > [2]https://github.com/haproxy/haproxy/blob/ > a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 > > Best regards > Tim Düsterhus > > > References: > > [1] mailto:t...@bastelstu.be > [2] > https://github.com/haproxy/haproxy/blob/a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 -- ~Nico
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
well, I met some projects that are insane on formatting. it is funny each time, when discussing new PR "please fix your formatting" (weeks spent on that), after PR is merged something is broken because of lack of testing. I'd say it is friendly mood not to push people to comply formatting (who cares of indentation ?) чт, 17 дек. 2020 г. в 13:05, Tim Düsterhus : > Ilya, > > Am 17.12.20 um 08:26 schrieb Илья Шипицин: > > gcc 11 (delivered with fedora rawhide) introduces indentation check. > > it is totally harmless. > > > > this patch suppresses it. > > it resolves #998 > > might be backported to all supported branches. > > > > Can we please fix the actual issue instead of suppressing the warning? > In fact I believe that this line is incorrectly intended: > > > https://github.com/haproxy/haproxy/blob/a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 > > Best regards > Tim Düsterhus >
Re: [PATCH} suppress "misleading indentation" gcc 11 warning
Ilya, Am 17.12.20 um 08:26 schrieb Илья Шипицин: > gcc 11 (delivered with fedora rawhide) introduces indentation check. > it is totally harmless. > > this patch suppresses it. > it resolves #998 > might be backported to all supported branches. > Can we please fix the actual issue instead of suppressing the warning? In fact I believe that this line is incorrectly intended: https://github.com/haproxy/haproxy/blob/a4009cd6103a92752db27c3a85051c6adcc832c1/include/import/plock.h#L236 Best regards Tim Düsterhus
