Re: svn commit: r362217 - head/stand/common
[ Charset UTF-8 unsupported, converting... ]
> On Tue, Jun 16, 2020 at 8:33 PM Ian Lepore wrote:
>
> > On Tue, 2020-06-16 at 19:34 +0200, Kristof Provost wrote:
> > > On 16 Jun 2020, at 19:11, Ed Maste wrote:
> > > > On Tue, 16 Jun 2020 at 13:01, Ian Lepore wrote:
> > > > >
> > > > > As much as I prefer doing it this way, style(9) doesn't allow for
> > > > > variable declarations inside a for() statement (or even inside a
> > > > > local
> > > > > block, which is just too 1980s for me, but it is still our standard).
> > > >
> > > > Perhaps it's time to update style(9) to at least permit these uses, as
> > > > we've done with the blank line at the beginning of functions with no
> > > > local variables, and with braces around single-line bodies.
> > >
> > > We have 431 instances of `for (int i` in sys alone. It?s not so much a
> > > question of allowing it as acknowledging reality at this point.
> > >
> > > Best regards,
> > > Kristof
> >
> > Hmm, so we do. If you weed out sys/contrib, and device drivers
> > contributed by vendors, the number is a lot smaller, but still big
> > enough that we should just change the rules I think.
> >
>
> We should definitely just change the rules. There's no point in
> prohibiting it. Contributors have already voted with their feet
>
> diff --git a/share/man/man9/style.9 b/share/man/man9/style.9
> index 4e801bbcbe70..fd23d573eb00 100644
> --- a/share/man/man9/style.9
> +++ b/share/man/man9/style.9
> @@ -592,8 +592,6 @@ not
> Parts of a
> .Ic for
> loop may be left empty.
> -Do not put declarations
> -inside blocks unless the routine is unusually complicated.
Perhaps some wording here that makes it explicit that
block scope variables are allowed, and that the for()
case is allowed.
> .Bd -literal
> for (; cnt < 15; cnt++) {
+ for (int cnt = 0; cnt < 15; cnt++) {
+ char *p;
> stmt1;
This updates the example to reflect the new accepted style.
>
> Although the block doesn't start until { so int i; in the commit
> technically doesn't violate this rule. We violate it in dozens of other
> ways than this.
I think it violates some other rule about declarations being
in order of size sorted at the top of a routine, perhaps that
needs looked at as well for some change.
> Warner
--
Rod Grimes rgri...@freebsd.org
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On Tue, 16 Jun 2020 at 18:09, Cy Schubert wrote: > > In message <55903c38d363aef2a6f6d0075dd4526b86d51258.ca...@freebsd.org>, > Ian Le > pore writes: > > On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote: > > > Author: tsoome > > > Date: Tue Jun 16 07:05:03 2020 > > > New Revision: 362217 > > > URL: https://svnweb.freebsd.org/changeset/base/362217 > > > > > > Log: > > > loader: variable i is unused without MBR/GPT support built in > > > > > > Because i is only used as index in for loop, declare it in for > > > statement. > > > > > > > As much as I prefer doing it this way, style(9) doesn't allow for > > variable declarations inside a for() statement (or even inside a local > > block, which is just too 1980s for me, but it is still our standard). > > Doesn't this use stack for a shorter period of time or does the compiler > optimize this, making this change moot? > > The tradeoff is a few extra bytes of stack for a longer period of time vs a > few extra instructions incrementing and decrementing the stack pointer. > > At least for LLVM it makes no difference where you declare the variable: it will always be represented by an alloca instruction at the start of the IR function: https://godbolt.org/z/NFG3hN There will also not be any changes to the stack pointer since the stack frame size is fixed no matter where the variables are declared (unless you use variable-length arrays or __builtin_alloca()). LLVM will also merge variables that have non-overlapping lifetimes to reuse the same stack slot if possible. I'm almost certain GCC performs the same optimizations (as will any other compiler written in the last 20 years). Alex ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On Tue, Jun 16, 2020 at 9:53 PM Warner Losh wrote:
>
>
> On Tue, Jun 16, 2020 at 8:33 PM Ian Lepore wrote:
>
>> On Tue, 2020-06-16 at 19:34 +0200, Kristof Provost wrote:
>> > On 16 Jun 2020, at 19:11, Ed Maste wrote:
>> > > On Tue, 16 Jun 2020 at 13:01, Ian Lepore wrote:
>> > > >
>> > > > As much as I prefer doing it this way, style(9) doesn't allow for
>> > > > variable declarations inside a for() statement (or even inside a
>> > > > local
>> > > > block, which is just too 1980s for me, but it is still our
>> standard).
>> > >
>> > > Perhaps it's time to update style(9) to at least permit these uses, as
>> > > we've done with the blank line at the beginning of functions with no
>> > > local variables, and with braces around single-line bodies.
>> >
>> > We have 431 instances of `for (int i` in sys alone. It’s not so much a
>> > question of allowing it as acknowledging reality at this point.
>> >
>> > Best regards,
>> > Kristof
>>
>> Hmm, so we do. If you weed out sys/contrib, and device drivers
>> contributed by vendors, the number is a lot smaller, but still big
>> enough that we should just change the rules I think.
>>
>
> We should definitely just change the rules. There's no point in
> prohibiting it. Contributors have already voted with their feet
>
> diff --git a/share/man/man9/style.9 b/share/man/man9/style.9
> index 4e801bbcbe70..fd23d573eb00 100644
> --- a/share/man/man9/style.9
> +++ b/share/man/man9/style.9
> @@ -592,8 +592,6 @@ not
> Parts of a
> .Ic for
> loop may be left empty.
> -Do not put declarations
> -inside blocks unless the routine is unusually complicated.
> .Bd -literal
> for (; cnt < 15; cnt++) {
> stmt1;
>
> Though the block doesn't start until { so int i; in the commit technically
> doesn't violate this rule. We violate it in dozens of other ways than this.
>
Re-reading the thread, it seems there's a consensus to change.
https://reviews.freebsd.org/D25312
> Warner
>
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On Tue, Jun 16, 2020 at 8:33 PM Ian Lepore wrote:
> On Tue, 2020-06-16 at 19:34 +0200, Kristof Provost wrote:
> > On 16 Jun 2020, at 19:11, Ed Maste wrote:
> > > On Tue, 16 Jun 2020 at 13:01, Ian Lepore wrote:
> > > >
> > > > As much as I prefer doing it this way, style(9) doesn't allow for
> > > > variable declarations inside a for() statement (or even inside a
> > > > local
> > > > block, which is just too 1980s for me, but it is still our standard).
> > >
> > > Perhaps it's time to update style(9) to at least permit these uses, as
> > > we've done with the blank line at the beginning of functions with no
> > > local variables, and with braces around single-line bodies.
> >
> > We have 431 instances of `for (int i` in sys alone. It’s not so much a
> > question of allowing it as acknowledging reality at this point.
> >
> > Best regards,
> > Kristof
>
> Hmm, so we do. If you weed out sys/contrib, and device drivers
> contributed by vendors, the number is a lot smaller, but still big
> enough that we should just change the rules I think.
>
We should definitely just change the rules. There's no point in
prohibiting it. Contributors have already voted with their feet
diff --git a/share/man/man9/style.9 b/share/man/man9/style.9
index 4e801bbcbe70..fd23d573eb00 100644
--- a/share/man/man9/style.9
+++ b/share/man/man9/style.9
@@ -592,8 +592,6 @@ not
Parts of a
.Ic for
loop may be left empty.
-Do not put declarations
-inside blocks unless the routine is unusually complicated.
.Bd -literal
for (; cnt < 15; cnt++) {
stmt1;
Although the block doesn't start until { so int i; in the commit
technically doesn't violate this rule. We violate it in dozens of other
ways than this.
Warner
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On Tue, 2020-06-16 at 10:30 -0700, Rodney W. Grimes wrote: > > In message < > > 55903c38d363aef2a6f6d0075dd4526b86d51258.ca...@freebsd.org>, > > Ian Le > > pore writes: > > > On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote: > > > > Author: tsoome > > > > Date: Tue Jun 16 07:05:03 2020 > > > > New Revision: 362217 > > > > URL: https://svnweb.freebsd.org/changeset/base/362217 > > > > > > > > Log: > > > > loader: variable i is unused without MBR/GPT support built in > > > > > > > > Because i is only used as index in for loop, declare it in > > > > for > > > > statement. > > > > > > > > > > As much as I prefer doing it this way, style(9) doesn't allow for > > > variable declarations inside a for() statement (or even inside a > > > local > > > block, which is just too 1980s for me, but it is still our > > > standard). > > Last time I tried to assert that bit of style(9) with respect to > inside a local block I was over ridden, so it is probably time > to atleast revisit that part of style(9). > > > Doesn't this use stack for a shorter period of time or does the > > compiler > > optimize this, making this change moot? > > > > The tradeoff is a few extra bytes of stack for a longer period of > > time vs a > > few extra instructions incrementing and decrementing the stack > > pointer. > > I do not think the reasoning had to do with stack space vs > instuctions, > but more about being able to clearly know about variable reuse and > not > do it as that can create fun bugs. > > Tools have modernized since that part of style was written. I think a modern compiler will likely emit one of those "declaration of foo here shadows declaration in outer scope" type messages. Not that it should; IMO, part of the point of keeping definitions confined to as local a scope as possible is to help ensure you're using the variable you think you are in that local scope and not accidentally perturbing something used elsewhere. -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On Tue, 2020-06-16 at 19:34 +0200, Kristof Provost wrote: > On 16 Jun 2020, at 19:11, Ed Maste wrote: > > On Tue, 16 Jun 2020 at 13:01, Ian Lepore wrote: > > > > > > As much as I prefer doing it this way, style(9) doesn't allow for > > > variable declarations inside a for() statement (or even inside a > > > local > > > block, which is just too 1980s for me, but it is still our standard). > > > > Perhaps it's time to update style(9) to at least permit these uses, as > > we've done with the blank line at the beginning of functions with no > > local variables, and with braces around single-line bodies. > > We have 431 instances of `for (int i` in sys alone. It’s not so much a > question of allowing it as acknowledging reality at this point. > > Best regards, > Kristof Hmm, so we do. If you weed out sys/contrib, and device drivers contributed by vendors, the number is a lot smaller, but still big enough that we should just change the rules I think. -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
> In message <55903c38d363aef2a6f6d0075dd4526b86d51258.ca...@freebsd.org>, > Ian Le > pore writes: > > On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote: > > > Author: tsoome > > > Date: Tue Jun 16 07:05:03 2020 > > > New Revision: 362217 > > > URL: https://svnweb.freebsd.org/changeset/base/362217 > > > > > > Log: > > > loader: variable i is unused without MBR/GPT support built in > > > > > > Because i is only used as index in for loop, declare it in for > > > statement. > > > > > > > As much as I prefer doing it this way, style(9) doesn't allow for > > variable declarations inside a for() statement (or even inside a local > > block, which is just too 1980s for me, but it is still our standard). Last time I tried to assert that bit of style(9) with respect to inside a local block I was over ridden, so it is probably time to atleast revisit that part of style(9). > Doesn't this use stack for a shorter period of time or does the compiler > optimize this, making this change moot? > > The tradeoff is a few extra bytes of stack for a longer period of time vs a > few extra instructions incrementing and decrementing the stack pointer. I do not think the reasoning had to do with stack space vs instuctions, but more about being able to clearly know about variable reuse and not do it as that can create fun bugs. Tools have modernized since that part of style was written. > -- > Cy Schubert -- Rod Grimes rgri...@freebsd.org ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On 16 Jun 2020, at 19:11, Ed Maste wrote: On Tue, 16 Jun 2020 at 13:01, Ian Lepore wrote: As much as I prefer doing it this way, style(9) doesn't allow for variable declarations inside a for() statement (or even inside a local block, which is just too 1980s for me, but it is still our standard). Perhaps it's time to update style(9) to at least permit these uses, as we've done with the blank line at the beginning of functions with no local variables, and with braces around single-line bodies. We have 431 instances of `for (int i` in sys alone. It’s not so much a question of allowing it as acknowledging reality at this point. Best regards, Kristof ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On 16/06/2020 12:01, Ian Lepore wrote: On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote: Author: tsoome Date: Tue Jun 16 07:05:03 2020 New Revision: 362217 URL: https://svnweb.freebsd.org/changeset/base/362217 Log: loader: variable i is unused without MBR/GPT support built in Because i is only used as index in for loop, declare it in for statement. As much as I prefer doing it this way, style(9) doesn't allow for variable declarations inside a for() statement (or even inside a local block, which is just too 1980s for me, but it is still our standard). Perhaps style(9) needs updating? I think KNF is mandatory for kernel code only, and is only suggested for userland. Pedro. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On Tue, 16 Jun 2020 at 13:01, Ian Lepore wrote: > > As much as I prefer doing it this way, style(9) doesn't allow for > variable declarations inside a for() statement (or even inside a local > block, which is just too 1980s for me, but it is still our standard). Perhaps it's time to update style(9) to at least permit these uses, as we've done with the blank line at the beginning of functions with no local variables, and with braces around single-line bodies. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
In message <55903c38d363aef2a6f6d0075dd4526b86d51258.ca...@freebsd.org>, Ian Le pore writes: > On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote: > > Author: tsoome > > Date: Tue Jun 16 07:05:03 2020 > > New Revision: 362217 > > URL: https://svnweb.freebsd.org/changeset/base/362217 > > > > Log: > > loader: variable i is unused without MBR/GPT support built in > > > > Because i is only used as index in for loop, declare it in for > > statement. > > > > As much as I prefer doing it this way, style(9) doesn't allow for > variable declarations inside a for() statement (or even inside a local > block, which is just too 1980s for me, but it is still our standard). Doesn't this use stack for a shorter period of time or does the compiler optimize this, making this change moot? The tradeoff is a few extra bytes of stack for a longer period of time vs a few extra instructions incrementing and decrementing the stack pointer. -- Cheers, Cy Schubert FreeBSD UNIX: Web: https://FreeBSD.org NTP: Web: https://nwtime.org The need of the many outweighs the greed of the few. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r362217 - head/stand/common
On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote: > Author: tsoome > Date: Tue Jun 16 07:05:03 2020 > New Revision: 362217 > URL: https://svnweb.freebsd.org/changeset/base/362217 > > Log: > loader: variable i is unused without MBR/GPT support built in > > Because i is only used as index in for loop, declare it in for > statement. > As much as I prefer doing it this way, style(9) doesn't allow for variable declarations inside a for() statement (or even inside a local block, which is just too 1980s for me, but it is still our standard). -- Ian ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r362217 - head/stand/common
Author: tsoome
Date: Tue Jun 16 07:05:03 2020
New Revision: 362217
URL: https://svnweb.freebsd.org/changeset/base/362217
Log:
loader: variable i is unused without MBR/GPT support built in
Because i is only used as index in for loop, declare it in for statement.
Sponsored by: Netflix, Klara Inc.
Modified:
head/stand/common/part.c
Modified: head/stand/common/part.c
==
--- head/stand/common/part.cTue Jun 16 04:17:08 2020(r362216)
+++ head/stand/common/part.cTue Jun 16 07:05:03 2020(r362217)
@@ -647,7 +647,6 @@ ptable_open(void *dev, uint64_t sectors, uint16_t sect
struct dos_partition *dp;
struct ptable *table;
uint8_t *buf;
- int i;
#ifdef LOADER_MBR_SUPPORT
struct pentry *entry;
uint32_t start, end;
@@ -720,7 +719,7 @@ ptable_open(void *dev, uint64_t sectors, uint16_t sect
* start sector 1. After DOSPTYP_PMBR, there may be other partitions.
* UEFI compliant PMBR has no other partitions.
*/
- for (i = 0; i < NDOSPART; i++) {
+ for (int i = 0; i < NDOSPART; i++) {
if (dp[i].dp_flag != 0 && dp[i].dp_flag != 0x80) {
DPRINTF("invalid partition flag %x", dp[i].dp_flag);
goto out;
@@ -742,7 +741,7 @@ ptable_open(void *dev, uint64_t sectors, uint16_t sect
/* Read MBR. */
DPRINTF("MBR detected");
table->type = PTABLE_MBR;
- for (i = has_ext = 0; i < NDOSPART; i++) {
+ for (int i = has_ext = 0; i < NDOSPART; i++) {
if (dp[i].dp_typ == 0)
continue;
start = le32dec(&(dp[i].dp_start));
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
