I’ve submitted a pull request to clean up the noted ‘visual pollution’ as well as a few other visually awkward constructions found in this module.

 

I don’t see any benefit in debating this further, and I’ll leave with these comments:

 

-          It may be important to the reader to understand that the file descriptor passed to both the write and ioctl functions are unused, since this modules calls these functions directly with a null file descriptor (bypassing the file system logic that would make use of said descriptors).

-          This was already indicated in the function’s documentation block (I put it there).

-          There is NO suitable and portable annotation for this.

 

From: px4dev [mailto:notifications@github.com] 
Sent: Saturday, August 17, 2013 8:33 PM
To: PX4/Firmware
Cc: Jean Cyr
Subject: Re: [Firmware] Flesh out PX4IO documentation comments and delete unnecessary class var (#361)

 

In src/drivers/px4io/px4io.cpp:

> @@ -1628,7 +1667,8 @@ class PX4IO : public device::I2C
>  }
>  
>  ssize_t
> -PX4IO::write(file *filp, const char *buffer, size_t len)
> +PX4IO::write(file * /*filp*/, const char *buffer, size_t len)
> +/* Make it obvious that file * isn't used here */

I understand what you've done; I'm asking that you not visually pollute the signature in the class definition with a non-obvious annotation linked to an implementation detail.

It is not important to the reader that this specific implementation of CDev::write doesn't use the argument. If it were, the place to note to the reader that the argument is unused is in the documentation block for the function. If it's important to note it to the compiler, the attribute should be placed near the implementation, so that when the implementation is changed the annotation can be removed.

An annotation of the kind you've used makes good sense when your readers are all familiar with it - as I noted, the fact that you felt it was necessary to explain the annotation to your reader (were you planning on doing that every time you did that, for every argument that isn't used in every method?) is a pretty good hint that it's not going to work.

The notation you've chosen is by no means unique to the meaning you've assigned, either; historically it was used to avoid namespace collisions between preprocessor macro definitions and function argument names, so it can easily be read more than one way...

—
Reply to this email directly or view it on GitHub <https://github.com/PX4/Firmware/pull/361/files#r5830223> .  <https://github.com/notifications/beacon/mZHFuVAGpjKMu8Zke4leWAICZGQ18f9Cyb9neeugRvGbjPG6Fe8_5QarQbIkBFLK.gif>