Fix Clang warnings by using proper function prototypes in macros

[ShravanDeva5327]

This PR updates the macros to specify (void) in argument lists for functions that take no arguments. This resolves warnings from
Clang(-Wstrict-prototypes).

Tested locally with Clang; no build issues observed.

Closes #178
Supersedes #115

[fujitatomoya]

@ShravanDeva5327 change looks good to me, but DCO is missing. can you address that?

[ShravanDeva5327]

@fujitatomoya I forgot to sign off the commit. I've added it now.

[christophebedard]

By the way, it's good practice to reference any relevant issue or PR in the PR description. This way, reviewers can get the whole context behind a change. This also applies when creating issues, of course.

In this case, since this PR supersedes #115 (and since my comments on that PR may be relevant for this PR), it would be good to include "Supersedes #115" on a separate line in the PR description.

[ShravanDeva5327]

By the way, it's good practice to reference any relevant issue or PR in the PR description. This way, reviewers can get the whole context behind a change. This also applies when creating issues, of course.

In this case, since this PR supersedes #115 (and since my comments on that PR may be relevant for this PR), it would be good to include "Supersedes #115" on a separate line in the PR description.

That makes sense, I’ve edited the PR description, and I’ll make sure to reference related issues or PRs in the future too

[christophebedard]

Oh, and you can also include "Closes #178" in the PR description (again on a separate line). This way, the #178 issue will automatically get closed when this PR gets merged!

[christophebedard]

I think there's a similar issue here: if I build up to test_tracetools with default options (so using gcc instead of clang, but I'm assuming the same thing happens with clang), I get a build error in rclcpp:

Starting >>> rclcpp
--- stderr: rclcpp                                                                          
In file included from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/service.hpp:35,
                 from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/callback_group.hpp:28,
                 from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/any_executable.hpp:20,
                 from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/memory_strategy.hpp:25,
                 from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/memory_strategies.hpp:18,
                 from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/executor_options.hpp:22,
                 from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/include/rclcpp/executor.hpp:38,
                 from /home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/src/rclcpp/executor.cpp:34:
/home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/src/rclcpp/executor.cpp: In member function ‘bool rclcpp::Executor::get_next_ready_executable(rclcpp::AnyExecutable&)’:
/home/christophe.bedard/ros2_ws/src/ros2/rclcpp/rclcpp/src/rclcpp/executor.cpp:776:3: error: expected primary-expression before ‘void’
  776 |   TRACETOOLS_TRACEPOINT(rclcpp_executor_get_next_ready);
      |   ^~~~~~~~~~~~~~~~~~~~~
gmake[2]: *** [CMakeFiles/rclcpp.dir/build.make:513: CMakeFiles/rclcpp.dir/src/rclcpp/executor.cpp.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....
gmake[1]: *** [CMakeFiles/Makefile2:498: CMakeFiles/rclcpp.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2
---
Failed   <<< rclcpp [18.1s, exited with code 2]

I see the same failure in this GitHub CI job: https://github.com/ros2/ros2_tracing/actions/runs/15215810484/job/42800943159?pr=179#step:5:2925.

It's because _TRACEPOINT_NOARGS and _DO_TRACEPOINT_NOARGS are used by TRACETOOLS_TRACEPOINT and TRACETOOLS_DO_TRACEPOINT (indirectly through _GET_MACRO_TRACEPOINT/_GET_MACRO_DO_TRACEPOINT). TRACETOOLS_TRACEPOINT and TRACETOOLS_DO_TRACEPOINT are used both in tracetools.c to define functions and by instrumented packages, which results in function calls with (void) and therefore fails the build (see above).

I think we have to create new macros just for the function name:

#  define _FUNC_TRACEPOINT(event_name) \
  (ros_trace_ ## event_name)
#  define _FUNC_TRACEPOINT_ENABLED(event_name) \
  (ros_trace_enabled_ ## event_name)
#  define _FUNC_DO_TRACEPOINT(event_name) \
  (ros_trace_do_ ## event_name)

and then those can be used by all other macros, where applicable, which also helps us avoid repeating the ros_trace_* ## event_name pattern in many locations. The macros that instrumented packages use should then not include (void), and the macros to define functions in tracetools.c can use (void).

[ShravanDeva5327]

I think I'm getting this now, the macros _TRACEPOINT_NOARGS / _DO_TRACEPOINT_NOARGS are being used to define (in tracetools.c) / call (in instrumented packages) functions which are already declared, with void explicitly specified, by _DECLARE_TRACEPOINT_NOARGS macros in tracetools.h. So undoing changes to L49 and L53 should work. Also did you suggest creating new macros to solve this issue specifically or as a general improvement?

[christophebedard]

I assumed that changing _TRACEPOINT_NOARGS/_DO_TRACEPOINT_NOARGS was required if we want to use (void) in the 2nd case described here #179 (comment), i.e., function definitions in tracetools.c. They use TRACETOOLS_TRACEPOINT/TRACETOOLS_DO_TRACEPOINT, which use _TRACEPOINT_NOARGS/_DO_TRACEPOINT_NOARGS. Since _TRACEPOINT_NOARGS/_DO_TRACEPOINT_NOARGS are used by TRACETOOLS_TRACEPOINT/TRACETOOLS_DO_TRACEPOINT, this means we can't just change them without adding (void) for the 3rd use-case described in #179 (comment). In that case, we'd have to create separate macros to be used for the function definitions in tracetools.c, without using the TRACETOOLS_TRACEPOINT/TRACETOOLS_DO_TRACEPOINT macros.

However, I think adding (void) for the 2nd use-case (function definition in tracetools.c) is not strictly necessary, so we could proceed with this. Did you check that there are no warnings reported by clang after your latest changes?

[christophebedard]

Did you check that there are no warnings reported by clang after your latest changes?

According to the latest nightly_linux_clang_libcxx job, it does complain about -Wstrict-prototypes in tracetools.c, although it's hard to tell which exact function it's complaining about. https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/2266/clang/folder.106785574/

[ShravanDeva5327]

Did you check that there are no warnings reported by clang after your latest changes?

Yes I did, and there were no warnings. also I used clang with -Wstrict-prototypes to compile a simple program

// test.h
int foo(void);

// test.c
#include "test.h"
int main(void) {
    foo();
    return 0;
}

int foo() {
    return 42;
}

this also didn't gave any warnings. So I assumed specifying (void) in function declaration is sufficient

[christophebedard]

Got it. Then I'll manually trigger a nightly_linux_clang_libcxx job with your branch and we'll see. ci.ros2.org isn't very responsive; as soon as I can trigger it, I'll post a link to the job here.

[christophebedard]

Here it is:

• nightly_linux_clang_libcxx

[christophebedard]

It unfortunately does complain about tracetools.c: https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/2267/clang/. Looking at the full warnings in the build output, it's definitely complaining about the function definitions: https://ci.ros2.org/view/nightly/job/nightly_linux_clang_libcxx/2267/console#console-section-163.

[ShravanDeva5327]

In that case I'll just add new macros you suggested, not entirely sure why I was not getting those warnings locally though.

[ShravanDeva5327]

This should hopefully resolve the warnings. Since I'm not able to reproduce them locally, I can't confirm if they're fully resolved.

@christophebedard Could you please trigger the CI job manually to check?