Remove the temporary variable in RCUTILS_LOGGING_AUTOINIT #290
Conversation
felixendres
force-pushed
the
patch-1
branch
from
3bc0f2b to
e3cef0c
Compare
include/rcutils/logging.h
Outdated
| } \ | ||
| } \ | ||
| } | ||
| } while (0); |
There was a problem hiding this comment.
This looks pretty good to me. One thing that I'd prefer to do here is to remove the semicolon from the end of this line, and make the callers of this macro have the semicolon. That will play more nicely with text editors, and also look more like statements inline.
To do this, we'd have to update all of the callers of RCUTILS_LOGGING_AUTOINIT to have a semicolon on the end. Most of the callers are in this repository, but there is also a caller in rcl and rclpy. We'd need to have separate PRs for that.
@felixendres Are you willing to make those updates? If so, please go ahead and fix up rcutils as suggested, and open PRs to rcl and rclpy as well. If not, that's OK, but please open an issue so we can do this follow-up in the future.
There was a problem hiding this comment.
I totally agree on @clalancette suggestion, in addition to
- No need to prevent uncrustify from making unnecessary indents.
- It is unlikely that user application calls
RCUTILS_LOGGING_AUTOINITdirectly.
include/rcutils/logging.h
Outdated
| } \ | ||
| } \ | ||
| } | ||
| } while (0); |
There was a problem hiding this comment.
I totally agree on @clalancette suggestion, in addition to
- No need to prevent uncrustify from making unnecessary indents.
- It is unlikely that user application calls
RCUTILS_LOGGING_AUTOINITdirectly.
|
I'd prefer to not make the semicolon changes. This issue is actually an extremely minor issue, so let's not sink too much time into it. Apart from the uncrustify comment (which I don't understand, so you be the judge) the current change improves the status quo. The semicolon issue is the same as before (putting one after the macro is superfluous before and after). So I suggest to merge it as is, unless that uncrustify thing is a problem. |
Fix issue ros2#285: Remove the temporary variable in a macro, because the name of the variable may shadow a variable of the macro user. As suggested by @clalancette, also wrapped it into a do-while wrapping. Signed-off-by: Felix Endres <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
|
I've gone ahead and rebased this, and also changed to remove the semicolon. Besides the changes in this PR, this required two other PRs, one in rclpy and one in rcl. Here's CI for the whole lot:
|
|
All right. The Windows failures are happening on nightlies as well, so this is ready to go. Since I made changes here, I'm going to wait for another review from @ros2/team . |
|
@felixendres it would good to update the ticket title to express the content of the patch since it will become the squashed commit message. |
clalancette
changed the title
felixendres
changed the title
felixendres
changed the title
|
Sorry, for the title-change back and forth. I could swear I still saw the old title "Update logging.h", when I changed it... |
No worries, it was probably just cached in your browser. |
5 participants
Fix issue #285:
Remove the temporary variable in a macro, because the name of the variable may shadow a variable of the macro user.
As suggested by @clalancette, also wrapped it into a do-while wrapping.
Signed-off-by: Felix Endres