charlesvdv / boulot-robot

GNU General Public License v3.0
0 stars 2 forks source link

Add clang-tidy support #28

Closed charlesvdv closed 4 years ago

charlesvdv commented 4 years ago

The most controversial rule I see is modernize-use-trailing-return-type which modifies things like this:

int f1();
inline int f2(int arg) noexcept;
virtual float f3() const && = delete;

// transforms to:

auto f1() -> int;
inline auto f2(int arg) -> int noexcept;
virtual auto f3() const && -> float = delete;

Personally, I quite like the syntax but if you prefer the more standard C++98 syntax. I don't see any issues to revert this rule.

charlesvdv commented 4 years ago

@JonathanPetit can you review the change again as it contains your last PR #27. At the same time, I took the liberty to remove some .hpp from the include and put them in the src folders as those files should not be exposed in the public API (it's always a good practice to expose the minimum as possible).

charlesvdv commented 4 years ago

The move semantic is pretty hard to explain in a short manner. So if you have some time you can check this SO answer.

The reason why I pass by value the string is coming from this clang-tidy rule. The idea is to be explicit about the copy operation being done.

With the previous solution (and the one I recommended before reading about this rule), you passed the value by reference and then copied it into the object. The solution proposed is to explicitly show to the API the copy operation and then move the value into the class. As the move operation is pretty cheap for string, it's almost the same as the previous solution (performance wise) but we make it clear to the caller that the value is being copied.

// Previous way
Foo::Foo(const std::string& val): //passed by reference. The caller don't know that we will copy the value
  val(val) {} // copy is done here

// New way
Foo::Foo(std::string val): // passed by value. The caller knows the string will be copied.
  val(std::move(val)) {} // move the string to avoid another copy

I hope this is clearer now?