davids5 commented on issue #12: nxstyle improvements with No tooling URL: https://github.com/apache/incubator-nuttx/pull/12#issuecomment-569512690 @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](https://github.com/apache/incubator-nuttx/pull/12#issuecomment-569422728) `-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](https://github.com/apache/incubator-nuttx/blob/master/tools/README.txt#L297-L318) 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](https://github.com/mikr/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](https://www.youtube.com/watch?v=9pFvM21Ng14) @0:398 ish seconds that comes to mind often in theses discussion..."If You Were Right, I Would Agree With You"
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
