Refine const function and references, remove unnecessary copy initializations

[KvanTTT]

No description provided.

[lightvector]

Thanks! This took a little while to review. I ended up accepting some of these, along with some minor edits of my own, in 92ad583.

Some of these I did not accept since they add const to methods that should not be const. For example clearCache() is semantically a mutating operation and should not be const, and the change to filesystem.hpp deviates from the official C++17 interface for filesystem (that method does not appear to be const) - the root cause of this appears to be that C++ does not propagate or require propagation of const through pointers, so it is sometimes possible to mark a method that mutates the internal state of an object as const, if that mutation of internal state is restricted to the contents of pointers within that object, but even though the rules of the language allow it, doing so is often confusing or inappropriate.

Others I did not accept because of the possibility of subtle bugs - for example if you eliminate a copy of an object and instead take a const reference, you now have to care about the lifetime of that reference, and also you have to check whether the referred object is potentially itself aliased or mutated anywhere such that now the mutation carries through the const reference and changes the behavior. I mostly went conservative and only accepted the ones I could verify easily. There are probably not bugs in the vast majority of the ones that weren't accepted, but given the very large number of altered sites it's hard to verify and be sure so I just went with the clearer and easier ones. Let me know if you know of a more intelligent way of applying and reviewing such refactors that can guard against such issues.

[KvanTTT]

Thanks! This took a little while to review. I ended up accepting some of these, along with some minor edits of my own, in 92ad583.

Thanks! I was not sure about legality of all changes, that's why I split the changes by commits. The most important were Apply "Pass value parameters by const reference" and Fix remaining unnecessary copy initializations because they might affect performance unlike other ones (that just improves strictness), so they could have been skipped.

Anyway, I'm satisfied with your commit because the most problematic cases were covered. For example, Board, Rules, SearchParams or vectors look too big to copy on each call that might be in a hot-path (I assume they might be because they are used in program/ and search/ directories).

Others I did not accept because of the possibility of subtle bugs - for example if you eliminate a copy of an object and instead take a const reference, you now have to care about the lifetime of that reference, and also you have to check whether the referred object is potentially itself aliased or mutated anywhere such that now the mutation carries through the const reference and changes the behavior.

I agree, they might be subtle.

Let me know if you know of a more intelligent way of applying and reviewing such refactors that can guard against such issues.

I'm not an expert in C++ ecosystem (well, I'm from Kotlin world), but I suppose Clang-Tidy can be configured in a way to skip some rules that can provoke false positives. Also, probably some C++ attributes might be useful (to suppress some warnings). I'll let you know if I find a robust solution.

[KvanTTT]

Let me know if you know of a more intelligent way of applying and reviewing such refactors that can guard against such issues.

@lightvector you can use the following syntax to suppress specific warnings on a certain string:

Board b = board; // NOLINT(performance-unnecessary-copy-initialization)

The NOLINT without parameters suppresses all warnings:

Board b = board; // NOLINT

Also, it's possible to suppress entire blocks:

// NOLINTBEGIN(readability-non-const-parameter, cppcoreguidelines-pro-bounds-pointer-arithmetic)
legacyCall(
  buffer,
  size
);
// NOLINTEND(readability-non-const-parameter, cppcoreguidelines-pro-bounds-pointer-arithmetic)

I've created an MR that configures clang-tidy and enables checks from #1167. Now those warnings (performance-inefficient-vector-operation , performance-unnecessary-copy-initialization, readability-non-const-parameter, readability-make-member-function-const) should be reported during compilation (unless they are suppressed).