-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
+ Coverage 40.95% 41.77% +0.81%
==========================================
Files 28 28
Lines 967 881 -86
Branches 566 505 -61
==========================================
- Hits 396 368 -28
+ Misses 54 53 -1
+ Partials 517 460 -57
Continue to review full report at Codecov.
|
@@ -29,7 +29,7 @@ namespace system_metrics_collector | |||
PeriodicMeasurementNode::PeriodicMeasurementNode( | |||
const std::string & name, | |||
const std::chrono::milliseconds measurement_period, | |||
const std::string & publishing_topic, | |||
const std::string & publishing_topic, // NOLINT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we fix this? Generally, each NOLINT must contain a justification of why the linting rules has been disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue the linter caught here was https://clang.llvm.org/extra/clang-tidy/checks/modernize-pass-by-value.html
I am not sure it warrants an API change. Any thoughts, @zmichaels11, @dabonnie, @emersonknapp?
I tried providing a justification with // NOLINT(modernize-pass-by-value)
, which clang-tidy
is happy about but cpplint
is not (it complained about unrecognized NOLINT
criteria).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add another comment explaining the // NOLINT
modernize-pass-by-value
is also something we should probably look more into as a team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am thinking we disregard modernize-pass-by-value
(turn off modernize-pass-by-value
checks) until we look more into it as a team, so NOLINT
will not be applicable here for now.
Note: this should be merged to master directly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be OOO until 08/01. I think Emerson, Davin and Aaron can give you good advice regarding clang-tidy.
system_metrics_collector/src/system_metrics_collector/linux_process_memory_measurement_node.cpp
Outdated
Show resolved
Hide resolved
system_metrics_collector/src/system_metrics_collector/linux_process_memory_measurement_node.cpp
Show resolved
Hide resolved
@@ -35,7 +35,7 @@ constexpr const char kMemAvailable[] = "MemAvailable:"; | |||
constexpr const char kEmptyFile[] = ""; | |||
constexpr const int kInvalidMemorySample = -1; | |||
|
|||
double ComputeCpuTotalTime(const ProcCpuData measurement1, const ProcCpuData measurement2) | |||
double ComputeCpuTotalTime(const ProcCpuData & measurement1, const ProcCpuData & measurement2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dabonnie I think this is done on purpose because those are lightweight objects, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I made a comment on this last time suggesting pass by val because ProcCpuData
has a size of 2 words. I'll re-check
Edit:
ProcCpuData
now holds an std::string
, so it should be passed by const ref.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was some confusing about ProcCpuData
and ProcPidCpuData
.
af6da6e
to
4f6c229
Compare
a9c6e83
to
321b1f9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, let's make sure with @dabonnie that we can safely modify computeCpuTotalTime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only nit to update the linux_process_memory_measurement_node.cpp
with a comment of why struct
is needed (so it doesn't keep coming up in review).
321b1f9
to
d4fac45
Compare
Rebased and added some changes to unit test source codes that were somehow missed earlier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't see any issues with proposed changes so LGTM
@@ -57,7 +57,7 @@ class TestLinuxCpuMeasurementNode : public system_metrics_collector::LinuxCpuMea | |||
const std::chrono::milliseconds publish_period) | |||
: LinuxCpuMeasurementNode(name, measurement_period, publishing_topic, publish_period) {} | |||
|
|||
virtual ~TestLinuxCpuMeasurementNode() = default; | |||
~TestLinuxCpuMeasurementNode() override = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stylistic choice of clang-tidy
's (declaring destructors as override
) is a controversial one: isocpp/CppCoreGuidelines#721.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I think the majority in the discussion sided with using override
as being practically good, even if the behavior/semantics of a virtual destructor isn't quite the same as that of a virtual function.
CodeCov Action is failing for unrelated reason - fix is tracked in https://github.com/ros-security/aws-oncall/issues/48. |
Fix linting issues found by
clang-tidy
6.0.