* alternative form flag with zero octal value @ 2018-01-11 0:43 Peter Wang 2018-01-11 2:02 ` Rich Felker 0 siblings, 1 reply; 8+ messages in thread From: Peter Wang @ 2018-01-11 0:43 UTC (permalink / raw) To: musl Hi, I'm not certain it is a bug, but this program produces "00" instead of "0": #include <stdio.h> int main(void) { printf("%#o\n", 0); return 0; } Please Cc: any replies as I am not subscribed to this list. Peter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alternative form flag with zero octal value 2018-01-11 0:43 alternative form flag with zero octal value Peter Wang @ 2018-01-11 2:02 ` Rich Felker 2018-01-11 2:17 ` Rich Felker 0 siblings, 1 reply; 8+ messages in thread From: Rich Felker @ 2018-01-11 2:02 UTC (permalink / raw) To: Peter Wang; +Cc: musl On Thu, Jan 11, 2018 at 11:43:05AM +1100, Peter Wang wrote: > Hi, > > I'm not certain it is a bug, but this program produces "00" instead of "0": > > #include <stdio.h> > int main(void) > { > printf("%#o\n", 0); > return 0; > } > > Please Cc: any replies as I am not subscribed to this list. Indeed, this is a bug. I'm not sure whether it was introduced in commit 78897b0dc00b7cd5c29af5e0b7eebf2396d8dce0 or already present, but it was not present in Dmitry Levin's original more complex version of the patch. I'm going to essentially revert this commit and replace it with an alternate one-line fix using infrastructure that wasn't available at the time but added later in commit 167dfe9672c116b315e72e57a55c7769f180dffa. Thanks for the report. Rich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alternative form flag with zero octal value 2018-01-11 2:02 ` Rich Felker @ 2018-01-11 2:17 ` Rich Felker 2018-01-11 8:12 ` Jens Gustedt 0 siblings, 1 reply; 8+ messages in thread From: Rich Felker @ 2018-01-11 2:17 UTC (permalink / raw) To: Peter Wang; +Cc: musl [-- Attachment #1: Type: text/plain, Size: 970 bytes --] On Wed, Jan 10, 2018 at 09:02:46PM -0500, Rich Felker wrote: > On Thu, Jan 11, 2018 at 11:43:05AM +1100, Peter Wang wrote: > > Hi, > > > > I'm not certain it is a bug, but this program produces "00" instead of "0": > > > > #include <stdio.h> > > int main(void) > > { > > printf("%#o\n", 0); > > return 0; > > } > > > > Please Cc: any replies as I am not subscribed to this list. > > Indeed, this is a bug. I'm not sure whether it was introduced in > commit 78897b0dc00b7cd5c29af5e0b7eebf2396d8dce0 or already present, > but it was not present in Dmitry Levin's original more complex version > of the patch. I'm going to essentially revert this commit and replace > it with an alternate one-line fix using infrastructure that wasn't > available at the time but added later in commit > 167dfe9672c116b315e72e57a55c7769f180dffa. > > Thanks for the report. Here's the patch I'm going to push if I don't find any problems with it. Rich [-- Attachment #2: 0001-fix-printf-alt-form-octal-with-value-0-and-no-explic.patch --] [-- Type: text/plain, Size: 1830 bytes --] From b64539ae06aa91a407359238f4e909adb9bfab3d Mon Sep 17 00:00:00 2001 From: Rich Felker <dalias@aerifal.cx> Date: Wed, 10 Jan 2018 20:45:02 -0500 Subject: [PATCH] fix printf alt-form octal with value 0 and no explicit precision commit 78897b0dc00b7cd5c29af5e0b7eebf2396d8dce0 wrongly simplified Dmitry Levin's original submitted patch fixing alt-form octal with the zero flag and field width present, omitting the special case where the value is zero. as a result, printf("%#o",0) wrongly prints "00" rather than "0". the logic prior to this commit was actually better, in that it was aligned with how the alt-form flag (#) for printf is specified ("it shall increase the precision"). at the time there was no good way to avoid the zero flag issue with the old logic, but commit 167dfe9672c116b315e72e57a55c7769f180dffa added tracking of whether an explicit precision was provided. revert commit 78897b0dc00b7cd5c29af5e0b7eebf2396d8dce0 and switch to using the explicit precision indicator for suppressing the zero flag. --- src/stdio/vfprintf.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c index 15356f5..50fb55c 100644 --- a/src/stdio/vfprintf.c +++ b/src/stdio/vfprintf.c @@ -559,7 +559,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, if (0) { case 'o': a = fmt_o(arg.i, z); - if ((fl&ALT_FORM) && p<z-a+1) prefix+=5, pl=1; + if ((fl&ALT_FORM) && p<z-a+1) p=z-a+1; } if (0) { case 'd': case 'i': pl=1; @@ -574,7 +574,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg, a = fmt_u(arg.i, z); } if (xp && p<0) goto overflow; - if (p>=0) fl &= ~ZERO_PAD; + if (xp) fl &= ~ZERO_PAD; if (!arg.i && !p) { a=z; break; -- 2.10.0 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alternative form flag with zero octal value 2018-01-11 2:17 ` Rich Felker @ 2018-01-11 8:12 ` Jens Gustedt 2018-01-11 18:55 ` Rich Felker 0 siblings, 1 reply; 8+ messages in thread From: Jens Gustedt @ 2018-01-11 8:12 UTC (permalink / raw) Cc: musl [-- Attachment #1: Type: text/plain, Size: 737 bytes --] Hello, seeing this patch, I am somewhat horrified by the coding style that is applied in that file. Comma operator, seriously? Conditionals without proper indentation? "else" at the end of a line and the depending statement in the next case? If have nothing against clever use of switch cases for such complicated case analysis, but the coding style should prominently make this clear and not obfuscate. Thanks Jens -- :: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS ::: :: ::::::::::::::: office Strasbourg : +33 368854536 :: :: :::::::::::::::::::::: gsm France : +33 651400183 :: :: ::::::::::::::: gsm international : +49 15737185122 :: :: http://icube-icps.unistra.fr/index.php/Jens_Gustedt :: [-- Attachment #2: Digitale Signatur von OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alternative form flag with zero octal value 2018-01-11 8:12 ` Jens Gustedt @ 2018-01-11 18:55 ` Rich Felker 2018-01-11 19:36 ` Jens Gustedt 0 siblings, 1 reply; 8+ messages in thread From: Rich Felker @ 2018-01-11 18:55 UTC (permalink / raw) To: musl On Thu, Jan 11, 2018 at 09:12:29AM +0100, Jens Gustedt wrote: > Hello, > seeing this patch, I am somewhat horrified by the coding style that is > applied in that file. > > Comma operator, seriously? Conditionals without proper indentation? > "else" at the end of a line and the depending statement in the next > case? > > If have nothing against clever use of switch cases for such > complicated case analysis, but the coding style should prominently > make this clear and not obfuscate. Yes, there is a good deal of stuff in that file that, in hindsight, I would do somewhat differently. A few of the worst things you noted could be 'fixed' trivially with gotos or compound statatements and probably should be. I don't want a big patch for this file that's hard to review/validate though; the return on time spent is just too small. If anyone does want to push me on making improvements here, small isolated/individual changes that can easily be seen to be correct are the way to go. Rich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alternative form flag with zero octal value 2018-01-11 18:55 ` Rich Felker @ 2018-01-11 19:36 ` Jens Gustedt 2018-01-12 0:38 ` Rich Felker 0 siblings, 1 reply; 8+ messages in thread From: Jens Gustedt @ 2018-01-11 19:36 UTC (permalink / raw) Cc: musl [-- Attachment #1: Type: text/plain, Size: 1557 bytes --] Hello Rich, On Thu, 11 Jan 2018 13:55:34 -0500 Rich Felker <dalias@libc.org> wrote: > Yes, there is a good deal of stuff in that file that, in hindsight, I > would do somewhat differently. A few of the worst things you noted > could be 'fixed' trivially with gotos No, I don't think this is necessary. I don't mind jumping in the middle of an "if (0)" branch, as long as it is indented correcly :) > or compound statatements and > probably should be. I don't want a big patch for this file that's hard > to review/validate though; the return on time spent is just too small. > If anyone does want to push me on making improvements here, small > isolated/individual changes that can easily be seen to be correct are > the way to go. Just a patch for white space and indentation would be easy, I think. - have break at the end of case lines, not at the start - start a newline before else - ident according to musl's coding style As long as git diff --color-words shows nothing, this should be fine. I could such a patch, if you want. Replacing comma operators by compound statements would be a bit more work. We could delay that to when somebody touches that file for more serious reasons. Jens -- :: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS ::: :: ::::::::::::::: office Strasbourg : +33 368854536 :: :: :::::::::::::::::::::: gsm France : +33 651400183 :: :: ::::::::::::::: gsm international : +49 15737185122 :: :: http://icube-icps.unistra.fr/index.php/Jens_Gustedt :: [-- Attachment #2: Digitale Signatur von OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alternative form flag with zero octal value 2018-01-11 19:36 ` Jens Gustedt @ 2018-01-12 0:38 ` Rich Felker 2018-01-12 8:54 ` Jens Gustedt 0 siblings, 1 reply; 8+ messages in thread From: Rich Felker @ 2018-01-12 0:38 UTC (permalink / raw) To: musl On Thu, Jan 11, 2018 at 08:36:23PM +0100, Jens Gustedt wrote: > Hello Rich, > > On Thu, 11 Jan 2018 13:55:34 -0500 Rich Felker <dalias@libc.org> wrote: > > > Yes, there is a good deal of stuff in that file that, in hindsight, I > > would do somewhat differently. A few of the worst things you noted > > could be 'fixed' trivially with gotos > > No, I don't think this is necessary. I don't mind jumping in the > middle of an "if (0)" branch, as long as it is indented correcly :) I'm not sure we're on the same page about what's bad then. I think if (1) something; else case bar: something_else; is gratuitously less readable than: something; goto shared_tail; case bar: something_else; shared_tail: Ideally we wouldn't need to do either, but sometimes it's hard to organized the code in a way that's not gratuitously redundant (at the source level) or gratuitously larger without stuff like this. > > or compound statatements and > > probably should be. I don't want a big patch for this file that's hard > > to review/validate though; the return on time spent is just too small. > > If anyone does want to push me on making improvements here, small > > isolated/individual changes that can easily be seen to be correct are > > the way to go. > > Just a patch for white space and indentation would be easy, I think. I'm not a fan of patches that just recreate the output of an indention utility. > - have break at the end of case lines, not at the start I guess you mean the pop_arg function? It's written the way it is because that's the form that makes it the easiest to see what's happening in each case and what's different between them. Putting the break on a separate line would make it take up a lot more space and lower the visual SNR. Putting it at the end of the line (without a lot of alignment space; such space could be an alternative option) would butt rendundant text up against the part that differs, making it harder to read too. One appproach that might satisfy us both is using a macro, something like: #define POP_CASE(a,b,c) case a: arg->b = va_arg(*ap, c); break; obviously with more meaningful names for the args. Then each line of the switch only contains meaningful information. > - start a newline before else > - ident according to musl's coding style I think a misunderstanding/different expectation you and perhaps others have is that a "coding style" is a deterministic function from source token sequences (and possibly some additional metadata like presence of blank lines) to a canonical source file. Some projects do things this way, but it's never been something I've liked. To me, "coding style" means a set of formatting constraints that should usually be met, unless there's a good reason to break them, but which don't determine a unique form, plus guidelines for how to favor readability when there's ambiguity about the best form. > As long as git diff --color-words shows nothing, this should be fine. > > I could such a patch, if you want. I'd really rather spend time reviewing & merging functional patches and implementing new things than discussing coding style. As I said before, if there are individual/isolated changes that can be accepted or deferred or rejected without requiring a long thread of v2, v3, ..., v10 patches until we agree on which combination of changes is right, I'm happy to commit the ones I agree with right away, but I'd still rather be spending time on something that feels more productive. > Replacing comma operators by compound statements would be a bit more > work. We could delay that to when somebody touches that file for more > serious reasons. Perhaps. Sometimes I'm not sure whether I prefer changing this sort of thing at the same time as a functional patch or doing it preemptively so that later functional changes are slimmer and easier to understand. Rich ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alternative form flag with zero octal value 2018-01-12 0:38 ` Rich Felker @ 2018-01-12 8:54 ` Jens Gustedt 0 siblings, 0 replies; 8+ messages in thread From: Jens Gustedt @ 2018-01-12 8:54 UTC (permalink / raw) Cc: musl [-- Attachment #1: Type: text/plain, Size: 4685 bytes --] Hello Rich, On Thu, 11 Jan 2018 19:38:11 -0500 Rich Felker <dalias@libc.org> wrote: > I'm not sure we're on the same page about what's bad then. I think > > if (1) something; else > case bar: > something_else; > > is gratuitously less readable than: > > something; > goto shared_tail; > case bar: > something_else; > shared_tail: Depends, I think non-case labels are disruptive if there are too many in the same function. A more clearly structured code, but with compound { } would also clearly tell where the flow goes. > Ideally we wouldn't need to do either, but sometimes it's hard to > organized the code in a way that's not gratuitously redundant (at the > source level) or gratuitously larger without stuff like this. sure > > > or compound statatements and > > > probably should be. I don't want a big patch for this file that's > > > hard to review/validate though; the return on time spent is just > > > too small. If anyone does want to push me on making improvements > > > here, small isolated/individual changes that can easily be seen > > > to be correct are the way to go. > > > > Just a patch for white space and indentation would be easy, I > > think. > > I'm not a fan of patches that just recreate the output of an indention > utility. > > > - have break at the end of case lines, not at the start > > I guess you mean the pop_arg function? It's written the way it is > because that's the form that makes it the easiest to see what's > happening in each case and what's different between them. Putting the > break on a separate line would make it take up a lot more space and > lower the visual SNR. Putting it at the end of the line (without a lot > of alignment space; such space could be an alternative option) would > butt rendundant text up against the part that differs, making it > harder to read too. > > One appproach that might satisfy us both is using a macro, something > like: > > #define POP_CASE(a,b,c) case a: arg->b = va_arg(*ap, c); break; > > obviously with more meaningful names for the args. Then each line of > the switch only contains meaningful information. > > > - start a newline before else > > - ident according to musl's coding style > > I think a misunderstanding/different expectation you and perhaps > others have is that a "coding style" is a deterministic function from > source token sequences (and possibly some additional metadata like > presence of blank lines) to a canonical source file. Some projects do > things this way, but it's never been something I've liked. To me, > "coding style" means a set of formatting constraints that should > usually be met, unless there's a good reason to break them, but which > don't determine a unique form, plus guidelines for how to favor > readability when there's ambiguity about the best form. For the grammatical part of "coding style" I effectively see it as more or less mechanical. Allowing for exceptions is not helping the occasional reader who at the same time has to apprehend the cleverness of the layout and of the code itself. This vfprintf code is an excellent example of that point. Just indenting it with the correct tabs makes it better readable for me. > > As long as git diff --color-words shows nothing, this should be > > fine. > > > > I could such a patch, if you want. > > I'd really rather spend time reviewing & merging functional patches > and implementing new things than discussing coding style. As I said > before, if there are individual/isolated changes that can be accepted > or deferred or rejected without requiring a long thread of v2, v3, > ..., v10 patches until we agree on which combination of changes is > right, I'm happy to commit the ones I agree with right away, but I'd > still rather be spending time on something that feels more productive. > > > Replacing comma operators by compound statements would be a bit more > > work. We could delay that to when somebody touches that file for > > more serious reasons. > > Perhaps. Sometimes I'm not sure whether I prefer changing this sort of > thing at the same time as a functional patch or doing it preemptively > so that later functional changes are slimmer and easier to understand. Let's forget about this then. Jens -- :: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS ::: :: ::::::::::::::: office Strasbourg : +33 368854536 :: :: :::::::::::::::::::::: gsm France : +33 651400183 :: :: ::::::::::::::: gsm international : +49 15737185122 :: :: http://icube-icps.unistra.fr/index.php/Jens_Gustedt :: [-- Attachment #2: Digitale Signatur von OpenPGP --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-01-12 8:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-11 0:43 alternative form flag with zero octal value Peter Wang 2018-01-11 2:02 ` Rich Felker 2018-01-11 2:17 ` Rich Felker 2018-01-11 8:12 ` Jens Gustedt 2018-01-11 18:55 ` Rich Felker 2018-01-11 19:36 ` Jens Gustedt 2018-01-12 0:38 ` Rich Felker 2018-01-12 8:54 ` Jens Gustedt
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).