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

Fix function erroneously declared as static #37

Merged
merged 1 commit into from Dec 11, 2019

Conversation

thomas-moulard
Copy link
Member

Functions declared in headers should never be static.
In the previous implementation, statisticsDataToString was duplicated
in each compilation unit as the function was both defined in the header
and declared as static.

@mm318
Copy link
Member

mm318 commented Dec 11, 2019

How about make it a member function of StatisticData?

@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

Merging #37 into master will not change coverage.
The diff coverage is 16.66%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #37   +/-   ##
=======================================
  Coverage   42.04%   42.04%           
=======================================
  Files          26       27    +1     
  Lines         880      880           
  Branches      506      506           
=======================================
  Hits          370      370           
  Misses         50       50           
  Partials      460      460
Flag Coverage Δ
#unittests 42.04% <16.66%> (ø) ⬆️
Impacted Files Coverage Δ
..._collector/src/moving_average_statistics/types.hpp 100% <ø> (+71.42%) ⬆️
..._collector/src/moving_average_statistics/types.cpp 16.66% <16.66%> (ø)
...ollector/linux_process_memory_measurement_node.hpp 100% <0%> (ø) ⬆️
...ollector/linux_process_memory_measurement_node.cpp 33.33% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf9bb2f...f79f702. Read the comment docs.

@thomas-moulard
Copy link
Member Author

thomas-moulard commented Dec 11, 2019

It's generally better to avoid fixing bugs and refactoring in the same PR. I don't have a strong opinion one way or another (except that a header called types.hpp should not have functions defined). My goal here was to try to get rid of the partial hit on the function. I thought it was because of the issue with static but it seems I was wrong. We can still merge this I guess as this is a step in the right direction (?)

edit: that being said I just dropped the spuriousstd::to_string in this function, so whatever works for me.

edit2: the tests relies on a char-for-char diff of the string so I can't get rid of std::to_string, back to only moving that into a cpp file.

@thomas-moulard thomas-moulard marked this pull request as ready for review December 11, 2019 17:48
Functions declared in headers should never be static.
In the previous implementation, statisticsDataToString was duplicated
in each compilation unit as the function was both defined in the header
and declared as static.

Signed-off-by: Thomas Moulard <tmoulard@amazon.com>
@@ -35,6 +35,7 @@ find_package(rcutils REQUIRED)

add_library(system_metrics_collector SHARED
src/moving_average_statistics/moving_average.cpp
src/moving_average_statistics/types.cpp
src/moving_average_statistics/types.hpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .hpp file should not be here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://stackoverflow.com/questions/36174499/why-add-header-files-into-add-library-add-executable-command-in-cmake

Some people prefer that style because then you get the headers in the auto-generated IDE solutions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's the intention here, otherwise this is a consistency issue, because why would this be the only header file under add_library()?

@thomas-moulard thomas-moulard merged commit 22a6ac2 into master Dec 11, 2019
@thomas-moulard thomas-moulard deleted the tmoulard/fix-static branch December 11, 2019 19:46
@thomas-moulard
Copy link
Member Author

I don't understand why the partial hit is still there - this is why the coverage tests is failing. Merging as I don't know how long it will take to understand the root cause here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants