cpplint: Enforce line_length on C-style comments #31
Conversation
Note that this adds false positives for whitespace/todo when TODO is used within a C-style comment. We don't care, because we have that warning disabled in Drake.
|
Er, actually, I'm not sure if I understand why we need this? Can you point to a spot in the Drake codebase or a Drake PR where you were bitten by this? (Also, just to make sure, this doesn't conflict with the rules set out in #30, does it?) |
|
This change diagnosed all of these problems: RobotLocomotion/drake#13762. Prior to this patch, cpplint ignores C-style comments entirely, so in Drake they had trailing whitespace, overly long lines, etc. This is not at odds with #30. This PR is only enabling linting of C-style comments. Where we use C vs C++ style, Doxygen style, or indentation, are unaffected. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)
a discussion (no related file):
Should this be blocked once it doesn't trigger errors on Drake? (the 300-character long lines mentioned in Slack convo)?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Should this be blocked once it doesn't trigger errors on Drake? (the 300-character long lines mentioned in Slack convo)?
- blocked until it doesn't [...]
|
Yup; that's why I haven't assigned the second reviewer yet. |
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers (waiting on @jwnimmer-tri)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
- blocked until it doesn't [...]
Will use RobotLocomotion/drake#13804 to show pass/fail of Drake.
There was a problem hiding this comment.
+@sherm1 for platform review per rotation, please.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform) (waiting on @EricCousineau-TRI and @sherm1)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Will use RobotLocomotion/drake#13804 to show pass/fail of Drake.
Passing now.
There was a problem hiding this comment.
Dismissed @EricCousineau-TRI from a discussion.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)
There was a problem hiding this comment.
Platform
Q: is there automatic support for NOLINT(readability/multiline_comment)?
Reviewed 2 of 2 files at r1.
Reviewable status:complete! all discussions resolved, LGTM from assignees EricCousineau-TRI(platform),sherm1(platform)
|
The NOLINT probably works out of the box, though I haven't tried it. We've never had to use it before, nor on master currently. |
Prior to this patch, cpplint ignores C-style comments entirely, so in Drake they had trailing whitespace, overly long lines, etc.
Note that this adds false positives for
whitespace/todowhen TODO is used within a C-style comment. We don't care, because we have that warning disabled in Drake.Tested vs Drake in RobotLocomotion/drake#13804.
This change is