possible nxstyle false alarm: "Missing space before closing C comment"

[apache]

possible nxstyle false alarm: "Missing space before closing C comment"

[xiaoxiang781216]

nxstyle claims "Missing space before closing C comment" on lines like the following:

      cpu  = sched_cpu_select(ALL_CPUS /* ptcb->affinity */);

in sched/sched/sched_mergepending.c.
i don't understand what it isn't happy about.

Change to?

      cpu  = sched_cpu_select(ALL_CPUS ); /* ptcb->affinity */

[yamt]

@xiaoxiang781216
are you suggesting this is not a false alarm?
if so, i guess the message "Missing space before closing C comment" needs an improvement.

[johannes-nivus]

I think it isn't a false alarm, it shows there is something wrong:
In case of comments only 3 types are specified,
"Single line", "Block" and "Right of code".
I don't think comments inside code statements should be encouraged.
But you're right: the error message is wrong.

[xiaoxiang781216]

I think it isn't a false alarm, it shows there is something wrong:
In case of comments only 3 types are specified,
"Single line", "Block" and "Right of code".
I don't think comments inside code statements should be encouraged.
But you're right: the error message is wrong.

I don't read the style guiding recently, not sure whether the guide require we have only three possible location to put the comment. My suggestion just try to make nxsytle happy:).
Anyway, if something isn't written in the document which mean the rule don't forbid we do so in the code. So nxstyle should be more conservative from my point of view.
We are all human, not a machine, right?:), we can make better decision most time if the engineer isn't a junior. Sometime the comment in the argument list may make it more clearer. For example, many function accept true/false to select the different strategy, adding the argument name as the comment before or after true/false make the code more readable.

[patacongo]

I don't think comments inside code statements should be encouraged

The coding standard is clear about any form of commented out code. It should be avoided unless there is explanation, then the may be disabled only with #if 0, never commented out.

[johannes-nivus]

The discussion isn't about commented out code, but about comments inside code, perhaps parameter lists:

      cpu  = sched_cpu_select(ALL_CPUS /* ptcb->affinity */);

[patacongo]

The discussion isn't about commented out code, but about comments inside code, perhaps parameter lists

Yes, but this case is violating the coding standard for other reasons and is fixed with #542

Please merge, someone. That is gating other PRs.

[patacongo]

This is the error:

At line 1474: line[n-1] == '*' and line[n] == '/' and n >= 2

This test at line 1474 is line[n+1] != ' ' and line[n-2] != '*'

That should be line[n-1] != ' ', right? (actually !isspace(line[n-1])). It is trying to verify that */ is preceded by a space like " */" or by another asterisk as in a block comment "**/" This is just a dumb coding error.

Do you want to fix it? Or should I? Let's not collide.

[johannes-nivus]

I see, but can you please share your opinion concerning comments inside statements that is not commented out code but perhaps an explanation?

call(int a /* This is an integer */ );

[patacongo]

I see, but can you please share your opinion concerning comments inside statements that is not commented out code but perhaps an explanation?

call(int a /* This is an integer */ );

If it is not prohibited, then it is permitted. Providing it does not conflict with common sense, acceptable practice. And if it conflicts with common sense, acceptable practice then it should be prohibited.

I don't have a strong opinion in this case. I would not comment in that way[1]. And would raise my eybrows to see such commenting.

Ultimately coding standards come down to aesthetics. But the coding standard must be commonly applied for good software quality. So those aesthetics need to be formalized and made standard .. cast in concrete. If they are not so formalized, then it is left as a matter of personal preference UNLESS you do want to formalilze it.

If you want to forbid comments in the argument lists, I will support that but then you should also submit a PR against Documentation/NuttXCCodingStandard.html. That is the master coding standard, the version on the website is a copy.

Greg

[1] CAVEAT: I don't normally comment this way, but I am the culprit in the example we were discussing. In this case, I just left some commented out stuff left over from debug. I still think that the commented out code is correct but it is commented out because it didn't work and I forgot to remove the comments.

In #542 I kept the question but moved it out of the argument list and marked it clearly with REVISIT:

[patacongo]

The issue here is just a nxstyle bug at line 1474. But thinking more about the larger question, "Should comments be allowed in parameter lists?" Ithink that the answer has to be YES. After reconsideration I realize that that is actually done quite often. The example case at hand is perverse, but parameter commenting like the following is very common and should be permitted (in both actual and formal parameters):

int foobar(int a,                    /* This is parameter a */
           int b,                    /* This is parameter b */
           int c);                   /* This is parameter c */

I have done that a few times myself when the values passed as actual parameters are not clear. It is probably not a good thing to do for formal parameters since these should be documented in the "Input Parameters" section of the function header, not in the parameter list. However, many people do put comments on formal parameters.

[patacongo]

A slightly more credible example:

draw_rectangle(y2 - y1 + 1,         /* Height */
               x2 - x1 + 1,         /* Width */
               (y2 -y1 + 1) *       /* Area */
               (x2 - x1 + 1));

In mathematical logic, sometimes the arguments can be relatively complex expressions mat not be intuitive and should be commented.

In the above case, I would probably use local variables to hold at least the height and width, but others do things differently. You get the general idea.

[johannes-nivus]

This is the error:

At line 1474: line[n-1] == '*' and line[n] == '/' and n >= 2

This test at line 1474 is line[n+1] != ' ' and line[n-2] != '*'

That should be line[n-1] != ' ', right? (actually !isspace(line[n-1])). It is trying to verify that */ is preceded by a space like " */" or by another asterisk as in a block comment "**/" This is just a dumb coding error.

Do you want to fix it? Or should I? Let's not collide.

I will fix it, give me one or two hours.

Concerning the comment stuff, I get the idea, and currently your examples shouldn't produce problems... but I will test.

[patacongo]

Closing... This should be fixed with #543