Global use of nodiscard

[CursedRock17]

This PR is meant to address #786 in the rosidl repo, so using nodiscard on any getter functions and operators if possible. Adding nodiscard should prevent a lot of unnecessary bugs ahead of time. Though, I do have a couple things that we may need to address

1. Is this something that we want to do in any repository with C++, since it would reduce bugs projectwide? If that's the case, this issue should be expanded to the ros2 repo, even if Jazzy is enduring a feature freeze.
2. To what extent should we be using nodiscard? At the original release of this PR I just went for getters and operators because of the implementation details:

Getters and operators could have the [[nodiscard]] attribute to prevent bugs.

Though realistically many functions could utilize nodiscard to prevent weird issues.

3. Any operator that returns*this cannot use nodiscard because you must always assign that this value but each operator overload has nodiscard, so we would have to make extra functions which don't use nodiscard, getting rid of the purpose.

[fujitatomoya]

lgtm.

IMO this is better. Once we can have a consensus to go this path, i can help this.

[clalancette]

In general, I'm for changes like this where we can do it; we have similar macros in C (https://github.com/ros2/rcutils/blob/2773c6069b844bc7866dc2199827784d0aac7f28/include/rcutils/macros.h#L25-L31).

That said, we need to run full CI with this in place. I think we'll end up finding a few places here and there that don't check the return value.

[clalancette]

1. Is this something that we want to do in any repository with C++, since it would reduce bugs projectwide? If that's the case, this issue should be expanded to the ros2 repo, even if Jazzy is enduring a feature freeze.

I think we want to do it on a repository-by-repository basis regardless, so this PR is a good start.

2. To what extent should we be using nodiscard? At the original release of this PR I just went for getters and operators because of the implementation details:

I'm fine with extending it as far as it makes sense.

3. Any operator that returns*this cannot use nodiscard because you must always assign that this value but each operator overload has nodiscard, so we would have to make extra functions which don't use nodiscard, getting rid of the purpose.

Yeah, so we wouldn't add nodiscard to these functions.

[clalancette]

Here's full CI with this in place:

• Linux
• Linux-aarch64
• Windows

[clalancette]

It looks to me like this is all happy with the downstream. And the issues on CI are known issues, unrelated to this PR. So I'm going to go ahead and merge this one in, thanks @CursedRock17

[clalancette]

It looks to me like this is all happy with the downstream. And the issues on CI are known issues, unrelated to this PR. So I'm going to go ahead and merge this one in, thanks @CursedRock17

Ah, sorry, scratch that.

So, it turns out that we cannot update the type_description/* files like this. In particular, those signatures have to match the ones that would have been generated by the rest of the rosidl pipeline. What I'm going to suggest here is that we actually just remove those changes from this PR, and go with the rest of them. In the future we can consider updating the type_description/* files when we update what the rosidl pipeline generates.

[CursedRock17]

Interesting, one thing I was contemplating on touching on was inside of the rosidl_generator_cpp, in msg__struct would adding nodiscard here add problems throughout the pipeline?

[clalancette]

Interesting, one thing I was contemplating on touching on was inside of the rosidl_generator_cpp, in msg__struct would adding nodiscard here add problems throughout the pipeline?

It's not necessarily a problem; I'd be OK with adding that. But we just have to make sure that the type_description/* files are updated in lockstep with whatever other changes we do.

Either way, I think we should do it in a separate PR; this one is almost good to go (just need to run CI on it one more time).

[clalancette]

Another round of full CI for this:

• Linux
• Linux-aarch64
• Windows