No CETL_NODISCARD for most of the public api #359
Conversation
…_session #verification
…#verification #docs #sonar
…#verification #docs #sonar
| [](https://sonarcloud.io/dashboard?id=UAVCAN_libuavcan) | ||
| [](https://sonarcloud.io/dashboard?id=UAVCAN_libuavcan) | ||
| [](https://sonarcloud.io/project/overview?id=OpenCyphal-Garage_libcyphal) | ||
| [](https://sonarcloud.io/project/overview?id=OpenCyphal-Garage_libcyphal) |
There was a problem hiding this comment.
When time comes we of course will quickly search/replace "OpenCyphal-Garage" to → "OpenCyphal" everywhere in this repo.
|
|
||
| /// @brief Takes the next payload fragment (aka CAN frame) from the reception queue unless it's empty. | ||
| /// | ||
| /// @param payload_buffer The payload of the frame will be written into the mutable `payload_buffer` (aka span). | ||
| /// @return Description of a received fragment if available; otherwise an empty optional is returned immediately. | ||
| /// `nodiscard` is used to prevent ignoring the return value, which contains not only possible media error, | ||
| /// but also important metadata (like `payload_size` field for further parsing of the result payload). |
There was a problem hiding this comment.
IMO this is the only public method which has little sense with discarding its result. All other methods could be used without checking results (f.e. if you don't care about possible errors and etc.) - anyway, no leaks or dangerous behavior will happen.
serges147
requested review from
pavel-kirienko,
lydia-at-amazon and
thirtytwobits
| /// @param params Various initial parameters for the message RX session (like subject id and extent size). | ||
| /// @return Either a unique interface pointer to the message RX session, or an error. | ||
| /// |
There was a problem hiding this comment.
| /// @param params Various initial parameters for the message RX session (like subject id and extent size). | |
| /// @return Either a unique interface pointer to the message RX session, or an error. | |
| /// |
Do these comments serve any purpose at all? It is evident from the type of the method that it takes params and returns either an interface unique ptr or an error, why spell it out? For better SNR we should only write things that are worth writing.
@thirtytwobits this is an example of an unhelpful doc comment; I suggest we avoid these.
There was a problem hiding this comment.
Yes, I need them for two purposes:
- Please see that, in addition to "obvious" stuff, there is also extra note about lifetime of result sessions regarding parent transport.
- Doxygen - docs generation is just bad without explicit mentioning of public functions.
- Last but not least - there is less confusion in rules what we document and what we don't - it's much easier to go with "always document public stuff" comparing to fuzzy "it is evident from ..."
There was a problem hiding this comment.
My objection applies only to the two lines I commented on -- the ones that begin with @param and @return; the rest is useful and should be kept. I think this addresses points 1 and 2.
There was a problem hiding this comment.
oh, ok, got it - will remove these pairs of lines.
Co-authored-by: Pavel Kirienko <pavel.kirienko@gmail.com>
Co-authored-by: Pavel Kirienko <pavel.kirienko@gmail.com>
|
`setLocalNodeId` does the job.
| CETL_NODISCARD std::size_t copy(const std::size_t offset_bytes, | ||
| void* const destination, | ||
| const std::size_t length_bytes) const | ||
| /// NOSONAR cpp:S5008 below currently unavoidable. Could be fixed if nunavut provides `cetl::byte*` support. |
There was a problem hiding this comment.
It looks like we're using cetl::byte* in other places in this PR now. Does this comment still apply?
There was a problem hiding this comment.
Yes, this comment still applies. Public facing ScatteredBuffer still uses void* as a second parameter. Internal stuff (aka detail) is switched to cetl::byte*.
|
nit: Title of PR: |
serges147
changed the title
|
|
||
| /// @brief Makes a message receive (RX) session. | ||
| /// | ||
| /// Lifetime of the RX session must never outlive this transport interface. |
There was a problem hiding this comment.
| /// Lifetime of the RX session must never outlive this transport interface. | |
| /// The RX session must never outlive this transport interface. |
|
No
CETL_NODISCARDfor the most of public api (issue #350), but private (akadetail) stuff stays under strict[[nodiscard]]policy for all non-voidreturning functions.Also:
NOSONAR-s by switching some internalvoid*-s →cetl::byte*-s.IMultiplexerat CAN - no use of it; but will be at UDP.README.md.