nxstyle improvements with No tooling

[davids5]

• Added features
  • outputs parse-able compiler like error format
  • Uses getops to parse command line.
  • Supports
    -s silence all output
    -g provide a PASS fail message

test script to check the whole of nuttx with @patacongo value of 86

#!/bin/bash

LIST=$(find ../ -type f \( -iname \*.c -o -iname \*.h \))
for f in $LIST; do 
    ./nxstyle -g -m 86 $f; 
done

Good news with length at 86, 5256 files pass.

./test.sh 2>&1 | grep PASSED | wc -l
5256 

BAD news with length at 86, 4043 files fail.

./test.sh  2>&1 | grep FAIL | wc -l
4043

Test cases

Test case 1

ironic but true
./nxstyle -m 99 nxstyle-master.c

nxstyle-master.c:316:11: error: Bad alignment
nxstyle-master.c:319:13: error: Bad left brace alignment
nxstyle-master.c:320:15: error: Bad comment alignment
nxstyle-master.c:321:16: error: Bad comment block alignment
nxstyle-master.c:322:16: error: Bad comment block alignment
nxstyle-master.c:323:16: error: Bad comment block alignment
nxstyle-master.c:325:15: error: Bad alignment
nxstyle-master.c:329:17: error: Bad left brace alignment
nxstyle-master.c:333:17: error: Bad right brace alignment
nxstyle-master.c:335:15: error: Bad comment alignment
nxstyle-master.c:336:16: error: Bad comment block alignment
nxstyle-master.c:337:16: error: Bad comment block alignment
nxstyle-master.c:338:16: error: Bad comment block alignment
nxstyle-master.c:339:16: error: Bad comment block alignment
nxstyle-master.c:341:15: error: Bad alignment
nxstyle-master.c:342:17: error: Bad left brace alignment
nxstyle-master.c:345:19: error: Bad alignment
nxstyle-master.c:346:21: error: Bad left brace alignment
nxstyle-master.c:347:21: error: Bad right brace alignment
nxstyle-master.c:349:19: error: Bad alignment
nxstyle-master.c:351:21: error: Bad left brace alignment
nxstyle-master.c:353:21: error: Bad right brace alignment
nxstyle-master.c:354:17: error: Bad right brace alignment
nxstyle-master.c:355:13: error: Bad right brace alignment
nxstyle-master.c:406:1: error: Missing blank line after comment
nxstyle-master.c:480:10: error: C++ style comment
nxstyle-master.c:540:1: error: Missing blank line after comment
nxstyle-master.c:609:0: error: C++ style comment
nxstyle-master.c:609:0: error: No indentation line
nxstyle-master.c:611:0: error: C++ style comment
nxstyle-master.c:611:0: error: No indentation line
nxstyle-master.c:617:0: error: C++ style comment
nxstyle-master.c:617:0: error: No indentation line
nxstyle-master.c:891:100: error: Long line found
nxstyle-master.c:1189:1: error: Too many blank lines
nxstyle-master.c:1211:56: error: Missing space before closing C comment
nxstyle-master.c:1330:1: error: Missing blank line before comment found
nxstyle-master.c:1356:1: error: Missing blank line before comment found
nxstyle-master.c:1405:1: error: Missing blank line before comment found
nxstyle-master.c:1446:1: error: Missing blank line before comment found
nxstyle-master.c:1474:105: error: Long line found
nxstyle-master.c:1501:1: error: Missing blank line before comment found
nxstyle-master.c:1516:1: error: Missing blank line before comment found
nxstyle-master.c:1544:1: error: Missing blank line before comment found
nxstyle-master.c:1572:1: error: Missing blank line before comment found
nxstyle-master.c:1592:1: error: Missing blank line before comment found
nxstyle-master.c:1607:1: error: Missing blank line before comment found
nxstyle-master.c:1626:1: error: Missing blank line before comment found
nxstyle-master.c:1744:38: error: Operator/assignment must be preceded with whitespace

Test case 2

blank.c
nxstyle -m 90 blank.c
45 lines of /n

...

./nxstyle -m 80 blank.c

blank.c:1:1: error: File begins with a blank line
blank.c:2:1: error: Too many blank lines
blank.c:3:1: error: Too many blank lines
blank.c:4:1: error: Too many blank lines
blank.c:5:1: error: Too many blank lines
blank.c:6:1: error: Too many blank lines
blank.c:7:1: error: Too many blank lines
blank.c:8:1: error: Too many blank lines
blank.c:9:1: error: Too many blank lines
blank.c:10:1: error: Too many blank lines
blank.c:11:1: error: Too many blank lines
blank.c:12:1: error: Too many blank lines
blank.c:13:1: error: Too many blank lines
blank.c:14:1: error: Too many blank lines
blank.c:15:1: error: Too many blank lines
blank.c:16:1: error: Too many blank lines
blank.c:17:1: error: Too many blank lines
blank.c:18:1: error: Too many blank lines
blank.c:19:1: error: Too many blank lines
blank.c:20:1: error: Too many blank lines
blank.c:21:1: error: Too many blank lines
blank.c:22:1: error: Too many blank lines
blank.c:23:1: error: Too many blank lines
blank.c:24:1: error: Too many blank lines
blank.c:25:1: error: Too many blank lines
blank.c:26:1: error: Too many blank lines
blank.c:27:1: error: Too many blank lines
blank.c:28:1: error: Too many blank lines
blank.c:29:1: error: Too many blank lines
blank.c:30:1: error: Too many blank lines
blank.c:31:1: error: Too many blank lines
blank.c:32:1: error: Too many blank lines
blank.c:33:1: error: Too many blank lines
blank.c:34:1: error: Too many blank lines
blank.c:35:1: error: Too many blank lines
blank.c:36:1: error: Too many blank lines
blank.c:37:1: error: Too many blank lines
blank.c:38:1: error: Too many blank lines
blank.c:39:1: error: Too many blank lines
blank.c:40:1: error: Too many blank lines
blank.c:41:1: error: Too many blank lines
blank.c:42:1: error: Too many blank lines
blank.c:43:1: error: Too many blank lines
blank.c:44:1: error: Too many blank lines
blank.c:45:1: error: Too many blank lines
blank.c:45:1: fatal: "Private/Public Functions" not found! File was not be checked

Test case 3

./nxstyle  -m 99 nxstyle.c`

Test case 4

./nxstyle -g -m 99 nxstyle.c 
nxstyle.c: PASSED nxstyle check

Test case 5

./nxstyle -g -m 99 nxstyle.c; echo $? 
nxstyle.c: PASSED nxstyle check
0

Test case 6

./nxstyle -g -m 99 nxstyle-master.c; echo $? 
nxstyle-master.c: FAILED nxstyle check
1

[xiaoxiang781216]

@liuguo09 please take a look.

[patacongo]

I have already review this in its previous incarnation. So I give my approval nw. I don't know if we can approvals from the other people on the list.
There is no workflow governing tools in nuttx/tools or apps/tools. C tools should follow C coding standard and the committer should satisfy themselves that the changes are functional. These change does not seem to affect the coding standard verification logic, only the form of error reporting so it is low risk.
So I would think that if/when you, @xiaoxiang781216 (and Haitao), are satisfied with the change, I think you should go ahead with the merge.
When there are changes to nxstyle that do affect the detection of problems, there is some very careful testing should be done to (1) assure that the change detects the problem and (2) does not introduce false alarms. I can share how I do that when the time is right.

[patacongo]

The use of '-m 99' in the testing is a little odd. I usuaully use -m 86. This probably raises some tool use questions. The coding standard says:

Line Width. Text should not extend past column 78 in the typical C source or header file. Sometimes the nature of the content of a file may require that the lines exceed this limit. This often occurs in header files with naturally long definitions. If the line width must extend 78 lines, then some wider line width may be used in the file provided that it is used consistently.

This should match the width of the block comments:

Block Comments. Each grouping in the file is separated with a block comment. The block comment consists of:

• A line that consists of the opening C comment (/*) followed by a series of asterisks extending to the length of the line (usually to column 78).
• The name of the grouping, starting at column 4. An asterisk preceives the name of the grouping in column 1.
• A line that consists of the closing C comment (*/) at the end of the line (usually column 78) preceded by a series of asterisks extending to column 1.

So picking any fixed value will cause long lines to be missed OR many invalid "long line" error reports. Ideally, we should parse the file first, look at the block comments, make sure that they are the same width, and set the maximum line width automatically.

[davids5]

@patacongo - would you please build and run this under windows.

[patacongo]

Just noticed... tools/README.txt also needs to be updated. I think just the old Usage comments need to be replaced with the new Usage comments.

[davids5]

Just noticed... tools/README.txt also needs to be updated. I think just the old Usage comments need to be replaced with the new Usage comments.

@patacongo - Thank you for noticing. It is added.
Did it run ok on Windows?

[patacongo]

It is not clear the the disinction between warning, error, and fatal makes any sense for coding standby. isn't is a boolean; The code complies to the standard or it does not. One non-compliance is no better or worse than another.
But I will leave that to Xiao Xiang and Haitao Liu to determine if they believe that that concept is somehow useful.
How dif you decide what is an warning, or an error, or fatal. Do you have some criteria for degree of meeting the coding standard?

[davids5]

How to use nxstyle with style

Eclipse with CDT (https://gnu-mcu-eclipse.github.io/) Empowered development with SVD enabled debugging.

Setup Tab "Builder Setting"

Setup Tab "Behavior"

Tool in action

• select a file (open it)
• Select nxstyle as the tool
• The Clck the Hammer!

Get a list of CS violations

Click and fix them! (hint work bottom up)

[davids5]

@patacongo

First let me say I am Pro NuttX coding style. On Slack, I had stated something to the effect of: ...how incredibly readable code is that conforms to the NuttX coding style and to preserve your legacy, this must be part of the inviolables... I said it better there.

It is not clear the the disinction between warning, error, and fatal makes any sense for coding standby. isn't is a boolean; The code complies to the standard or it does not. One non-compliance is no better or worse than another.

Good questions!

Here is the short answer to your questions are:

it is fence:

  FILE *out = stdout;

  if (class > INFO)
    {
      out = stderr;
      g_status |= 1;
    }

How dif you decide what is an warning, or an error, or fatal. Do you have some criteria for degree of meeting the coding standard?

For the moment
ERROR("Long line found", lineno, n); is used in a c file
WARN("Long line found", lineno, n); is used in a h file
for maxline

See

          if (n > maxline)
            {
              if (g_file_type == C_SOURCE)
                {
                  ERROR("Long line found", lineno, n);
                }
              else if (g_file_type == C_HEADER)

                {
                  WARN("Long line found", lineno, n);
                }
            }

For a complete understanding.

grep -E "(FATAL|FATALFL|WARN|ERROR|INFO)\(\"" nxstyle.c | awk '{$1=$1;print}' | sort
ERROR("Bad alignment", lineno, indent);
ERROR("Bad comment alignment", lineno, indent);
ERROR("Bad comment block alignment", lineno, indent);
ERROR("Bad left brace alignment", lineno, indent);
ERROR("Bad left brace alignment", lineno, indent);
ERROR("Bad right brace alignment", lineno, indent);
ERROR("Blank line contains whitespace", lineno, 1);
ERROR("Blank line follows left brace", lineno, 1);
ERROR("Blank line precedes right brace at line",
ERROR("Block comment terminator must be on a "
ERROR("Carriage returns found. "
ERROR("Carriage returns found. "
ERROR("C comment opening on separate line", lineno, n);
ERROR("Closing C comment not indented", lineno, n);
ERROR("Closing without opening comment", lineno, n);
ERROR("C++ style comment", lineno, n);
ERROR("C++ style comment on at %d:%d\n",
ERROR("Dangling whitespace at the end of line", lineno, n);
ERROR("Expected indentation line", lineno, indent);
ERROR("File begins with a blank line", 1, 1);
ERROR("Functions begin", lineno, n);
ERROR("Garbage follows left bracket", lineno, n);
ERROR("Garbage follows right bracket",
ERROR("Garbage follows right bracket", lineno, n);
ERROR("Garbage on line after C comment", lineno, n);
ERROR("Insufficient indentation line", lineno, indent);
ERROR("Insufficient indentation", lineno, indent);
ERROR("Invalid character after asterisk "
ERROR("Left bracket not on separate line", lineno,
ERROR("Long line found", lineno, n);
ERROR("Missing asterisk in comment block", lineno, indent);
ERROR("Missing asterisk in comment", lineno, indent);
ERROR("Missing blank line after comment", comment_lineno,
ERROR("Missing blank line before comment found", lineno, 1);
ERROR("Missing file header comment block", lineno, 1);
ERROR("Missing space after opening C comment", lineno, n);
ERROR("Missing space before closing C comment", lineno,
ERROR("Missing whitespace after comma", lineno, n);
ERROR("Missing whitespace after keyword", lineno, n);
ERROR("Missing whitespace after keyword", lineno, n);
ERROR("Missing whitespace after keyword", lineno, n);
ERROR("Missing whitespace after semicolon", lineno, n);
ERROR("Mixed case identifier found", lineno, ident_index);
ERROR("Multiple data definitions", lineno, i + 1);
ERROR("Multiple data definitions on line",
ERROR("No indentation line", lineno, indent);
ERROR("No indentation line", lineno, indent);
ERROR("Operator/assignment must be followed with whitespace",
ERROR("Operator/assignment must be preceded "
ERROR("Operator/assignment must be preceded with whitespace",
ERROR("Operator/assignment must be preceded with whitespace",
ERROR("Right brace must be followed by a blank line",
ERROR("Right bracket not on separate line",
ERROR("right left brace alignment", lineno, indent);
ERROR("Small odd alignment", lineno, indent);
ERROR("Space follows left bracket", lineno, n);
ERROR("Space follows left parenthesis", lineno, n);
ERROR("Space precedes right bracket", lineno, n);
ERROR("Space precedes right parenthesis", lineno, n);
ERROR("Space precedes semi-colon",
ERROR("TABs found. First detected", lineno, n);
ERROR("TABs found. First detected", lineno, n);
ERROR("Too many blank lines", lineno, 1);
ERROR("Unmatched right brace", lineno, n);
ERROR("Unmatched right parentheses", lineno, n);
ERROR("Upper case hex constant found", lineno, ident_index);
ERROR("'while' must be on a separate line",
ERROR("Whitespace on blank line", lineno, indent);
ERROR("Whitespace on blank line", lineno, indent);
FATAL("Comment or string found at end of file", lineno, 1);
FATALFL("Failed to open", filename);
FATAL("\"Private/Public Functions\" not found!"
WARN("Long line found", lineno, n);
WARN("No file extension", 0 , 0);

[DISCUSSION]

It is not clear the the disinction between warning, error, and fatal makes any sense for coding standby. isn't is a boolean; The code complies to the standard or it does not. One non-compliance is no better or worse than another.

Agreed - a CS should be more digital than analog. But unfortunately the is not the current reality of the NuttX coding style or the current tools.

The above comment about -m is a good a example of it's analog nature.
Here is another example of it's analog nature.

If we want to grow the Apache NuttX project we need tooling and a CI that works for us not against us. (When we get to that juncture, I will post something about the poison of a bad CI implementations ) Suffice it to say: having a bad experience on any front, is death to a projects adoption!

Since we want pre-comment hooks and CI to check conformity to NuttX coding style. We have several issues to overcome.

A proficient NuttX coding style tool would check for conformance, suggest fixes and optionally apply them.

We do not have a proficient NuttX coding style tool or set of tools yet.

Many of us have had our countless hours of work destroyed by the indent.sh.

Remember to NEVER NEVER run indent.sh on a formatted file*

*Once you've been burned you will learn to commit often and rebase -i when it all looks OK.

Historical (see groups and lists) there are a lot posts like this:

I do see many problems with the code now that would not permit it come into the master branch. It is good you are looking at the coding standing. There are all also tools that can help you in nuttx/tools. You should apply these tools in the following order:

detab.c will replace all of the TABs wtih spaces. No TABs are allowing in .c or .h files.
rmcr.c will remove whitespace at the end of lines
convert-comments.c will replace all C++ style comments with C89 style comments as required by >the coding style
lowhex.c will convert upper case hex constants to lower case hex constants
indent.sh will get the code close to the NuttX coding system. If used on .h files, however, it tends to >add too much indentation. It also makes a few mistakes
nxstyle.c will do a final check to make sure that the code is close to the coding style.

If we want to grow the project. This can not continue to be the way we expect "embedded talent" to spend their time. It does not scale: ambiguity and non-deterministic processes wastes a great deal resources, time and effort and will result in many drive-by-pr's or forks.

The sensible conclusion is to remove the human from the loop and automate it with proper tools.

This change is the first step away from this quality of tooling. We have a long way to go.
I have work on a custom version of atsyle, and with @mubes suggestion of whatstyle but I have little time to get this all to converge.

It is my hope that this reporting change in this PR will give us more time to work on proper tooling,

I do feel strongly about it's ultimate integration:

The API should be a simple invocation with NO ROOM for analog setting or not checking what has been changed!

This got batted down previously:

make check_format
make format

but for all the right reasons make is the place it goes. Remember @patacongo 's argument that user is the customer. He is right and for a for a community to grow the barrier to entry need to be low.

Having a bad first experience is death to a projects adoption.

REMEMBER: The N00B can use the make command and the PRO can use the script - we are not forcing our 'will' on anyone we are enabling the community to grow and succeed.

There is a great line in Patch Adams @0:398 ish
seconds that comes to mind often in theses discussion..."If You Were Right, I Would Agree With You"

[patacongo]

This one is a little misleading:

FATAL("\"Private/Public Functions\" not found!"

There is a "switch" (a boolean) that changes the behavior when the tool reaches the code in the file. It detects this point whn the Private or Public Functions block comment is encountered. It is misleading on C files that contain only global data definitions. For thoses files, there will naturally be no such block comments and and perfectly fine code would generate this fatal error.

[patacongo]

@xiaoxiang781216 I have two functional improvements that I want to make to nxstyle.c, but I will wait for Xiao Xiang to dispose of this PR before I do those.

[davids5]

@patacongo. I'm glad you have some improvements. Your options are to: 1 wait OR 2 do a PR against this Branch in this repo OR 3 I give you write access to this repo and you can pull this branch and add your commits on it. David

…

On Sun, Dec 29, 2019, 7:18 AM patacongo ***@***.***> wrote: I have two functional improvements that I want to make to nxstyle.c, but I will wait for Xiao Xiang to dispose of this PR before I do those. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#12>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAO3BXML4P2HHF3T4SJK74LQ3C5TNANCNFSM4KACHTUQ> .

[liuguo09]

Changes look good. A typo error in README.txt, gor a given file may be for a given file?

[davids5]

Added changes indicated but @patacongo