[U-Boot] Question about Coding-Style
Hello,
I've read on the U-Boot website about coding style. They say 'All contributions to U-Boot should conform to the Linux kernel coding style'. Further i've read this link and there are at least 2 things which i have troubles with.
1) a tab-ident is 8 spaces (in eclipse for example with the built in K&R has 4 characters). With 8 characters per TAB the problem from point 2 enters very, very fast.
2) a line ist limited to 80 characters (maybe thus comes from very old days where displays couldn't show more than this and scrolling was very expensive). is this state of the art ?
In fact i've found a lot of files within the u-boot code which do not obey to this rules.
Now my question is, how strong are this two points ? checkpatch.pl rails against a patch file with a lot of warnings if there is a line longer than 80bytes, also it takes TABS as 8 spaces.
whats your opinion about this ?
Last question: Is this mailing list the right place of discussing such things ?
many thanks and best regards, Hannes
mfG Petermaier Hannes
Hi Hannes,
On 04/02/2014 13:50, Hannes Petermaier wrote:
Hello,
I've read on the U-Boot website about coding style. They say 'All contributions to U-Boot should conform to the Linux kernel coding style'. Further i've read this link and there are at least 2 things which i have troubles with.
a tab-ident is 8 spaces
Right
(in eclipse for example with the built in K&R has 4 characters). With 8 characters per TAB the problem from point 2 enters very, very fast.
a line ist limited to 80 characters (maybe thus comes from very old days where displays couldn't show more than this and scrolling was very expensive). is this state of the art ?
Yes, it is. Same rule as in kernel.
In fact i've found a lot of files within the u-boot code which do not obey to this rules.
I cannot tell for each exceptions you found. Maybe the code is very old, and it slipped into mainline without fixing the lenght. There are also some well-known exception (generally for tables), where a longer line was accepted after discussion in the ML to increase readability. (tables for pinmux, as example arch/arm/include/asm/arch-mx6/mx6dl_pins.h).
Now my question is, how strong are this two points ?
They are strong until there is a general acceptance on the ML to drop the rule. However, the rule was already discussed in the past (you can dig deep in archives), and the result was to maintain it, exactly as it is valid for linux.
checkpatch.pl rails against a patch file with a lot of warnings if there is a line longer than 80bytes, also it takes TABS as 8 spaces.
checkpatch is run before merging a patch into a merge tree by custodians. If checkpatch reports errors, they must be fixed.
whats your opinion about this ?
Last question: Is this mailing list the right place of discussing such things ?
I think you are in the right place to discuss these topics ;-)
Best regards, Stefano Babic
Hi
Il 04/feb/2014 15:15 "Stefano Babic" sbabic@denx.de ha scritto:
Hi Hannes,
On 04/02/2014 13:50, Hannes Petermaier wrote:
Hello,
I've read on the U-Boot website about coding style. They say 'All contributions to U-Boot should conform to the Linux kernel coding
style'.
Further i've read this link and there are at least 2 things which i have troubles with.
a tab-ident is 8 spaces
Right
(in eclipse for example with the built in K&R has 4 characters). With 8 characters per TAB the problem from point 2 enters very, very fast.
If it happens your code has two many nested block
Michael
a line ist limited to 80 characters (maybe thus comes from very old days where displays couldn't show more than this and scrolling was very expensive). is this state of the art ?
Yes, it is. Same rule as in kernel.
In fact i've found a lot of files within the u-boot code which do not
obey
to this rules.
I cannot tell for each exceptions you found. Maybe the code is very old, and it slipped into mainline without fixing the lenght. There are also some well-known exception (generally for tables), where a longer line was accepted after discussion in the ML to increase readability. (tables for pinmux, as example
arch/arm/include/asm/arch-mx6/mx6dl_pins.h).
Now my question is, how strong are this two points ?
They are strong until there is a general acceptance on the ML to drop the rule. However, the rule was already discussed in the past (you can dig deep in archives), and the result was to maintain it, exactly as it is valid for linux.
checkpatch.pl rails against a patch file with a lot of warnings if there is a line longer than 80bytes, also it takes TABS as 8 spaces.
checkpatch is run before merging a patch into a merge tree by custodians. If checkpatch reports errors, they must be fixed.
whats your opinion about this ?
Last question: Is this mailing list the right place of discussing such things ?
I think you are in the right place to discuss these topics ;-)
Best regards, Stefano Babic
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de ===================================================================== _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Stefano, many thanks.
This will be a hard challenge for me to obey to the rules :-)
The 80 character thing is reported from checkpatch.pl as WARNING, not as ERROR - so there will be a chance of the patch to become accepted by a custodian.
In between i tried to reformat by Code to obey to these rules, but now checkpatch.pl tells me following:
WARNING: Avoid unnecessary line continuations #531: FILE: board/BuR/bur_tseries/board.c:155: + if (tps65217_voltage_update(TPS65217_DEFDCDC3, \
in real life the code section looks like this: /* Set DCDC3 (CORE) voltage to 1.125V */ if (tps65217_voltage_update(TPS65217_DEFDCDC3, \ TPS65217_DCDC_VOLT_SEL_1125MV)) { puts("tps65217_voltage_update failure\n"); return; }
If i don't do the line break at line 155 checkpatch.pl is disturbing me with the 80 character rule.
Another thing is linewrapping of output strings, to obey to the rules i have to format the string as following:
if (i2c_probe(TPS65217_CHIP_PM)) { printf("PMIC chip (0x%02x) not present! skipping" \ "further configuration.\n", TPS65217_CHIP_PM); return; }
But this makes it impossible to grep the code in case of an error.
Any idea how to deal with such things ?
best regards, Hannes
Stefano Babic wrote:
Hi Hannes,
On 04/02/2014 13:50, Hannes Petermaier wrote:
Hello,
I've read on the U-Boot website about coding style. They say 'All contributions to U-Boot should conform to the Linux kernel coding style'. Further i've read this link and there are at least 2 things which i have troubles with.
a tab-ident is 8 spaces
Right
(in eclipse for example with the built in K&R has 4 characters). With 8 characters per TAB the problem from point 2 enters very, very fast.
a line ist limited to 80 characters (maybe thus comes from very old days where displays couldn't show more than this and scrolling was very expensive). is this state of the art ?
Yes, it is. Same rule as in kernel.
In fact i've found a lot of files within the u-boot code which do not obey to this rules.
I cannot tell for each exceptions you found. Maybe the code is very old, and it slipped into mainline without fixing the lenght. There are also some well-known exception (generally for tables), where a longer line was accepted after discussion in the ML to increase readability. (tables for pinmux, as example arch/arm/include/asm/arch-mx6/mx6dl_pins.h).
Now my question is, how strong are this two points ?
They are strong until there is a general acceptance on the ML to drop the rule. However, the rule was already discussed in the past (you can dig deep in archives), and the result was to maintain it, exactly as it is valid for linux.
checkpatch.pl rails against a patch file with a lot of warnings if there is a line longer than 80bytes, also it takes TABS as 8 spaces.
checkpatch is run before merging a patch into a merge tree by custodians. If checkpatch reports errors, they must be fixed.
whats your opinion about this ?
Last question: Is this mailing list the right place of discussing such things ?
I think you are in the right place to discuss these topics ;-)
Best regards, Stefano Babic
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de =====================================================================
Hi Hannes,
On 04/02/2014 15:50, Hannes Petermaier wrote:
The 80 character thing is reported from checkpatch.pl as WARNING, not as ERROR - so there will be a chance of the patch to become accepted by a custodian.
No, it will *not* be accepted. There must be a very good reason to accept it.
As far as I know, there are only a few warnings that are generally accepted (I think using typedf is one of them).
In between i tried to reformat by Code to obey to these rules, but now checkpatch.pl tells me following:
WARNING: Avoid unnecessary line continuations #531: FILE: board/BuR/bur_tseries/board.c:155:
- if (tps65217_voltage_update(TPS65217_DEFDCDC3, \
Why do you need "" ? It is not a macro, checkpatch is right.
in real life the code section looks like this: /* Set DCDC3 (CORE) voltage to 1.125V */ if (tps65217_voltage_update(TPS65217_DEFDCDC3, \ TPS65217_DCDC_VOLT_SEL_1125MV)) {
Again, you do not need the line continuation because it is not a macro. The compiler knows that the statement is not yet finished.
puts("tps65217_voltage_update failure\n"); return; }
If i don't do the line break at line 155 checkpatch.pl is disturbing me with the 80 character rule.
Simply drop the unnecessary "".
Another thing is linewrapping of output strings, to obey to the rules i have to format the string as following:
if (i2c_probe(TPS65217_CHIP_PM)) { printf("PMIC chip (0x%02x) not present! skipping" \ "further configuration.\n", TPS65217_CHIP_PM); return; }
But this makes it impossible to grep the code in case of an error.
You must combine a more complicate grep, maybe with the -A (after context) option or using a regexp. However, this is not a reason to break the rule.
Best regards, Stefano Babic
On Tue, Feb 04, 2014 at 04:02:56PM +0100, Stefano Babic wrote:
Hi Hannes,
On 04/02/2014 15:50, Hannes Petermaier wrote:
[snip]
Another thing is linewrapping of output strings, to obey to the rules i have to format the string as following:
if (i2c_probe(TPS65217_CHIP_PM)) { printf("PMIC chip (0x%02x) not present! skipping" \ "further configuration.\n", TPS65217_CHIP_PM); return; }
But this makes it impossible to grep the code in case of an error.
You must combine a more complicate grep, maybe with the -A (after context) option or using a regexp. However, this is not a reason to break the rule.
Strings are the reason to break the rule and we have checkpatch patched (mostly?) to not complain. It's even true in the kernel.
Hi Tom,
On Tue, 4 Feb 2014 10:07:32 -0500, Tom Rini trini@ti.com wrote:
On Tue, Feb 04, 2014 at 04:02:56PM +0100, Stefano Babic wrote:
Hi Hannes,
On 04/02/2014 15:50, Hannes Petermaier wrote:
[snip]
Another thing is linewrapping of output strings, to obey to the rules i have to format the string as following:
if (i2c_probe(TPS65217_CHIP_PM)) { printf("PMIC chip (0x%02x) not present! skipping" \ "further configuration.\n", TPS65217_CHIP_PM); return; }
But this makes it impossible to grep the code in case of an error.
You must combine a more complicate grep, maybe with the -A (after context) option or using a regexp. However, this is not a reason to break the rule.
Strings are the reason to break the rule and we have checkpatch patched (mostly?) to not complain. It's even true in the kernel.
Hmm... Last time I checked,
"abc" "def"
Is a valid C string, and does not require a backslash. Do I miss something?
Amicalement,
Hi Albert, Le 10/02/2014 10:58, Albert ARIBAUD a écrit :
Hi Tom,
On Tue, 4 Feb 2014 10:07:32 -0500, Tom Rinitrini@ti.com wrote:
On Tue, Feb 04, 2014 at 04:02:56PM +0100, Stefano Babic wrote:
Hi Hannes,
On 04/02/2014 15:50, Hannes Petermaier wrote:
[snip]
Another thing is linewrapping of output strings, to obey to the rules i have to format the string as following:
if (i2c_probe(TPS65217_CHIP_PM)) { printf("PMIC chip (0x%02x) not present! skipping" \ "further configuration.\n", TPS65217_CHIP_PM); return; }
But this makes it impossible to grep the code in case of an error.
You must combine a more complicate grep, maybe with the -A (after context) option or using a regexp. However, this is not a reason to break the rule.
Strings are the reason to break the rule and we have checkpatch patched (mostly?) to not complain. It's even true in the kernel.
Hmm... Last time I checked,
"abc" "def"
Is a valid C string, and does not require a backslash. Do I miss something?
Yes, you are of course correct: the result is the C string "abcdef" and the backslash in the original is superfluous. However a grep for "abcdef" in the source code won't match it :( This is why splitting strings like this is discouraged in the Linux kernel.
Cheers, Chris
--- Ce courrier électronique ne contient aucun virus ou logiciel malveillant parce que la protection avast! Antivirus est active. http://www.avast.com
Hi Chris,
Hmm... Last time I checked,
"abc" "def"
Is a valid C string, and does not require a backslash. Do I miss something?
Yes, you are of course correct: the result is the C string "abcdef" and the backslash in the original is superfluous. However a grep for "abcdef" in the source code won't match it :( This is why splitting strings like this is discouraged in the Linux kernel.
Got it, thanks.
Cheers, Chris
Amicalement,
Dear Chris,
In message 52F8A706.7030103@free.fr you wrote:
"abc" "def"
Is a valid C string, and does not require a backslash. Do I miss something?
Yes, you are of course correct: the result is the C string "abcdef" and =
the backslash in the original is superfluous. However a grep for "abcdef" in the source code won't match it :( This is why splitting strings like this is discouraged in the Linux kernel.
To be precise, the Linux CodingStyle refers to "user-visible strings such as printk messages".
Best regards,
Wolfgang Denk
Dear Hannes,
In message 29703.213.33.116.113.1391525447.squirrel@petermaier.org you wrote:
The 80 character thing is reported from checkpatch.pl as WARNING, not as ERROR - so there will be a chance of the patch to become accepted by a custodian.
It is better for your reputation ere on the list not to try to sneak poorly formatted code in. Just play by the rules, and these include to fix such warnings issues by checkpatch.
In between i tried to reformat by Code to obey to these rules, but now checkpatch.pl tells me following:
WARNING: Avoid unnecessary line continuations #531: FILE: board/BuR/bur_tseries/board.c:155:
- if (tps65217_voltage_update(TPS65217_DEFDCDC3, \
in real life the code section looks like this: /* Set DCDC3 (CORE) voltage to 1.125V */ if (tps65217_voltage_update(TPS65217_DEFDCDC3, \ TPS65217_DCDC_VOLT_SEL_1125MV)) { puts("tps65217_voltage_update failure\n"); return; }
If i don't do the line break at line 155 checkpatch.pl is disturbing me with the 80 character rule.
You can of course break the lines, but why do you put a backslash at the end of the line? It is this what checkpatch is complaining about: "unnecessary line continuations".
Another thing is linewrapping of output strings, to obey to the rules i have to format the string as following:
if (i2c_probe(TPS65217_CHIP_PM)) { printf("PMIC chip (0x%02x) not present! skipping" \ "further configuration.\n", TPS65217_CHIP_PM); return; }
No. printable strings shall not be broken, as you ay want to grep the source code for that very string you see in your console log.
But then, it might make sense to think if you can cose a shorter form of the text? In the above case, the line is > 60 characters long (BTW: you missed a space when breaking the string). Is this needed? For example:
printf("ERROR: PMIC at 0x%02x missing\n", TPS65217_CHIP_PM);
gives the same information, actually even more prominent in the boot log - and I don;t even have top break the line.
But this makes it impossible to grep the code in case of an error.
Any idea how to deal with such things ?
Be creative, and use common sense.
Best regards,
Wolfgang Denk
On Tue, Feb 04, 2014 at 12:50:18PM -0000, Hannes Petermaier wrote:
Hello,
I've read on the U-Boot website about coding style. They say 'All contributions to U-Boot should conform to the Linux kernel coding style'. Further i've read this link and there are at least 2 things which i have troubles with.
a tab-ident is 8 spaces (in eclipse for example with the built in K&R has 4 characters). With 8 characters per TAB the problem from point 2 enters very, very fast.
a line ist limited to 80 characters (maybe thus comes from very old days where displays couldn't show more than this and scrolling was very expensive). is this state of the art ?
In fact i've found a lot of files within the u-boot code which do not obey to this rules.
The exceptions to the line length rule are printed strings, as breaking them up can make debugging harder than it should be.
Now my question is, how strong are this two points ? checkpatch.pl rails against a patch file with a lot of warnings if there is a line longer than 80bytes, also it takes TABS as 8 spaces.
whats your opinion about this ?
Last question: Is this mailing list the right place of discussing such things ?
It's not debatable, really. It's important for a project to be consistent (as much as possible) in coding style, and if it helps, recall the old adage about a good compromise leaves everyone unhappy about something.
Dear Hannes,
In message 11685.213.33.116.116.1391518218.squirrel@petermaier.org you wrote:
I've read on the U-Boot website about coding style. They say 'All contributions to U-Boot should conform to the Linux kernel coding style'. Further i've read this link and there are at least 2 things which i have troubles with.
a tab-ident is 8 spaces (in eclipse for example with the built in K&R has 4 characters). With 8 characters per TAB the problem from point 2 enters very, very fast.
Please read the CodingStyle document completely. It says:
... and have the added benefit of warning you when you're nesting your functions too deep. Heed that warning.
a line ist limited to 80 characters (maybe thus comes from very old days where displays couldn't show more than this and scrolling was very expensive). is this state of the art ?
Yes, it is. Please read the CodingStyle document.
In fact i've found a lot of files within the u-boot code which do not obey to this rules.
That should not be that many files, resp. only such where there were good reasons for allowing for longer lines (like tables of GPIO initializations, not breaking printable strings, etc.). Of course, poorly formatted code can be found here and there - cleanup patches are always welcome.
Now my question is, how strong are this two points ? checkpatch.pl rails against a patch file with a lot of warnings if there is a line longer than 80bytes, also it takes TABS as 8 spaces.
whats your opinion about this ?
Please fix these issues before posting the patch.
Is this mailing list the right place of discussing such things ?
Sure.
Best regards,
Wolfgang Denk
On Tue, 2014-02-04 at 12:50 +0000, Hannes Petermaier wrote:
a line ist limited to 80 characters (maybe thus comes from very old days where displays couldn't show more than this and scrolling was very expensive). is this state of the art ?
Even on a fairly nice, modern screen, I find 80 columns to be about right for two side-by-side windows with a comfortable font size.
Plus, horizontal scrolling is still a pain[1], and excessively long lines can be harder to read even if they fit on the screen (this is why newspapers and other documents often format text in columns).
-Scott
[1] I'm looking at you, new boards.cfg format!
On Tue, Feb 4, 2014 at 8:03 PM, Scott Wood scottwood@freescale.com wrote:
Plus, horizontal scrolling is still a pain[1], and excessively long lines can be harder to read even if they fit on the screen (this is why newspapers and other documents often format text in columns).
-Scott
[1] I'm looking at you, new boards.cfg format!
I agree with you, Scott :-)
participants (9)
-
Albert ARIBAUD -
Chris Moore -
Fabio Estevam -
Hannes Petermaier -
Michael Trimarchi -
Scott Wood -
Stefano Babic -
Tom Rini -
Wolfgang Denk