WIP: clang-tidy fixes around const std::string references. #1764
Conversation
github-actions
bot
added
ABC
|
This change isn't quite ready to be merged yet as it currently includes changes to code under |
mithro
changed the title
| void* value = NULL; | ||
| auto v = this->my_map.find(key); | ||
| if (v != this->my_map.end()) { | ||
| value = v->second; | ||
| this->my_map.erase(v); | ||
| value = this->my_map[key]; |
There was a problem hiding this comment.
this is not a good strategy, the original v = v->second is what we want.
Because otherwise what we do here is to look up the value again which we already did before.
There was a problem hiding this comment.
I did a first quick scan, but these are a lot of files, and there are many questionable 'improvements' in there.
So I suggest to find the clang-tidy changes that only deal with std::string -> const std::string & in parameters as first step, which makes it easier to blanket review as it not contains hidden 'gems' of other things in there.
| value = v->second; | ||
| this->my_map.erase(v); | ||
| value = this->my_map[key]; | ||
| this->my_map.erase(key); |
There was a problem hiding this comment.
also, this also doesn't look like the right strategy. Here again we force the map to look up the value from the key.
But we already have the iterator. So here as well, the original code is better.
| void* value = NULL; | ||
| auto v = this->my_map.find(key); | ||
| if (v != this->my_map.end()) | ||
| value = v->second; | ||
| value = this->my_map[key]; |
There was a problem hiding this comment.
Same as above, this should be the original v->second
|
|
||
| return value; | ||
| } | ||
|
|
||
| bool Hashtable::is_empty() { | ||
| return my_map.empty(); | ||
| return (my_map.size() == 0); |
There was a problem hiding this comment.
This as well should be the original code my_map.empty(), not call to size().
| void* value = NULL; | ||
| auto v = this->my_map.find(key); | ||
| if (v != this->my_map.end()) | ||
| value = v->second; | ||
| value = this->my_map[key]; | ||
|
|
||
| return value; | ||
| } | ||
|
|
||
| bool Hashtable::is_empty() { |
There was a problem hiding this comment.
We might want to make this is_empty() call a const method. As well Hashtable::get(). But I guess this is out of the scope for this change.
| @@ -520,7 +521,7 @@ void change_to_number_node(ast_node_t* node, VNumber number) { | |||
|
|
|||
| node->type = NUMBERS; | |||
| node->types.identifier = temp_ident; | |||
| node->types.vnumber = new VNumber(number); | |||
| node->types.vnumber = new VNumber(std::move(number)); | |||
There was a problem hiding this comment.
I'd probably keep changes that involve std::move in a separate change to keep it by const std::string &.
(I'd only use std::move if really needed. It more uglifies code otherwise)
| @@ -45,7 +45,7 @@ struct Abc_ManTime_t_ | |||
|
|
|||
| #define TOLERANCE 0.001 | |||
|
|
|||
| static inline int Abc_FloatEqual( float x, float y ) { return fabs(x-y) < TOLERANCE; } | |||
| static inline int Abc_FloatEqual( float x, float y ) { return fabsf(x-y) < TOLERANCE; } | |||
There was a problem hiding this comment.
The abc/ directory is probably also something that is just imported and we shouldn't touch too much.
| @@ -84,7 +85,7 @@ PowerCallibSize* PowerCallibInputs::get_entry_bound(bool lower, | |||
|
|
|||
| PowerSpicedComponent::PowerSpicedComponent(std::string component_name, | |||
| float (*usage_fn)(int num_inputs, float transistor_size)) { | |||
| name = component_name; | |||
| name = std::move(component_name); | |||
There was a problem hiding this comment.
instead of calling std::move here, we should pass a const std::string & as parameter, and just use assignment here.
| @@ -131,19 +132,19 @@ int RoutingToClockConnection::create_virtual_clock_network_sink_node( | |||
| */ | |||
|
|
|||
| void ClockToClockConneciton::set_from_clock_name(std::string clock_name) { | |||
| from_clock = clock_name; | |||
| from_clock = std::move(clock_name); | |||
There was a problem hiding this comment.
all these: instead of using std::move from a by-value parameter, we should use assignment and pass in a const std::string &
Description
This is the
clang-tidyfixes that @luk036 had in his pull request at #1078These changes make the code follow closer to best practices around using const std::string references.