Skip to content
This repository was archived by the owner on Jun 10, 2021. It is now read-only.

Fix linting issues #54

Merged
merged 3 commits into from
Jan 13, 2020
Merged

Fix linting issues #54

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53672b6
fix linting issues found by clang-tidy
mm318 Dec 23, 2019
fc8f768
address PR comment
mm318 Jan 9, 2020
d4fac45
fix linting issues found in tests
mm318 Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ..._metrics_collector/src/system_metrics_collector/linux_process_memory_measurement_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ constexpr const char kMetricName[] = "_memory_percent_used";
*/
double GetSystemTotalMemory()
{
struct sysinfo si;
// the following needs the struct keyword because sysinfo is also a function name
struct sysinfo si {};
const auto success = sysinfo(&si);
return success == -1 ? std::nan("") : static_cast<double>(si.totalram);
}
Expand All @@ -66,10 +67,10 @@ double LinuxProcessMemoryMeasurementNode::PeriodicMeasurement()
double p_mem;
try {
p_mem = static_cast<double>(GetProcessUsedMemory(statm_line));
} catch (std::ifstream::failure e) {
} catch (const std::ifstream::failure & e) {
RCLCPP_ERROR(
this->get_logger(), "caught %s, failed to GetProcessUsedMemory from line %s",
e.what(), file_to_read_);
e.what(), file_to_read_.c_str());
return std::nan("");
}
const auto total_mem = GetSystemTotalMemory();
Expand Down
2 changes: 1 addition & 1 deletion system_metrics_collector/src/system_metrics_collector/utilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@
// See the License for the specific language governing permissions and
// limitations under the License.

#include <time.h>
#include <unistd.h>

#include <chrono>
#include <cmath>
#include <ctime>
#include <fstream>
#include <limits>
#include <sstream>
Expand Down
2 changes: 1 addition & 1 deletion system_metrics_collector/test/system_metrics_collector/test_collector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class TestCollector : public system_metrics_collector::Collector
{
public:
TestCollector() = default;
virtual ~TestCollector() = default;
~TestCollector() override = default;
bool SetupStart() override
{
return true;
Expand Down
2 changes: 1 addition & 1 deletion system_metrics_collector/test/system_metrics_collector/test_linux_cpu_measurement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

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.

Copy link
Member Author

@mm318 mm318 Jan 11, 2020 •
edited
Loading

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.


// make this private method public for unit testing purposes
double PeriodicMeasurement() override
Expand Down
2 changes: 1 addition & 1 deletion system_metrics_collector/test/system_metrics_collector/test_linux_memory_measurement.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ class TestLinuxMemoryMeasurementNode : public system_metrics_collector::LinuxMem
: LinuxMemoryMeasurementNode(name, measurement_period, publishing_topic, publish_period),
measurement_index_(0) {}

virtual ~TestLinuxMemoryMeasurementNode() = default;
~TestLinuxMemoryMeasurementNode() override = default;

void SetTestString(const std::string & test_string)
{
Expand Down
4 changes: 2 additions & 2 deletions system_metrics_collector/test/system_metrics_collector/test_periodic_measurement_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class TestPeriodicMeasurementNode : public ::system_metrics_collector::PeriodicM
const std::string & publishing_topic,
const std::chrono::milliseconds publish_period)
: PeriodicMeasurementNode(name, measurement_period, publishing_topic, publish_period) {}
virtual ~TestPeriodicMeasurementNode() = default;
~TestPeriodicMeasurementNode() override = default;

int GetNumPublished() const
{
Expand Down Expand Up @@ -76,7 +76,7 @@ class TestPeriodicMeasurementNode : public ::system_metrics_collector::PeriodicM
++times_published_;
}

std::string GetMetricName() const
std::string GetMetricName() const override
{
return kTestMetricname;
}
Expand Down