Fix cppcheck performance checks

[carlpitt]

Also clang-formatted the edited files.

Edit:

$ cppcheck --version
Cppcheck 2.12.0
$ cppcheck --enable=performance -f --std=c++11 .

ninja/src/build.cc:526:3: performance: Variable 'lock_file_path_' is assigned in constructor body. Consider performing initialization in initialization list. [useInitializationList]
  lock_file_path_ = ".ninja_lock";
  ^

ninja/src/util.cc:924:14: performance: Ineffective call of function 'substr' because a prefix of the string is assigned to itself. Use replace() instead. [uselessCallsSubstr]
    result = result.substr(0, elide_size)
             ^

For the latter, after reading what it would be with result.replace(...), it seemed better to limit allocations and copies by reserving the length width and appending the necessary strings.

Used git clang-format rather than clang-format entire files.

[digit-google]

Please no whole clang-reformat of source files mixed with code changes, this makes everything very difficult to review.
And in practice, it also makes other PRs much harder to merge. Instead, use git clang-format which will limit the reformatting to the parts of the code that you modified.

It may be desirable to reformat the whole source tree with a single clang-format pass, but doing so should probably be done after in a single isolated PR, after announcing it on the mailing list to warn anyone that has an active PRs to rebase their changes on top of it after it is submitted. Other large scale changes (e.g. removing namespace std use) would be similar, e.g. only convert from string to std::string on code that you add or modify, and leave the rest as is.

What is the purpose of this patch? in particular what cppcheck performance check is this supposed to address?
That should be part of the commit message.

Also, what is the practical benefit here? Performance optimization should be justified with numbers that show they improve a measurable metric for reasonable use cases. E.g. it looks like you somehow optimize ElideMiddle() by reserving a string buffer but that probably doesn't impact Ninja build times in any reasonable way, so I don't see a reason to accept your PR here unless you can show that this improves things for users, instead of a small micro-optimization.

[carlpitt]

@digit-google First of all, thank you for the feedback. I was not aware of git clang-format.

I would certainly be willing to do both a full clang-format pass as well as remove all instances of using namespace std;. Ought I make a mailing list post before submitting the PRs for those, or just before the PRs are accepted?

Finally, I edited the PR description. Please tell me if you need more.

[digit-google]

Thanks for your changes, a few more notes though:

• Please squash your two commits into a single one for clarity.

• Put the information in the commit message, instead of the PR conversation. The former will be available to anyone cloning the repository, including many forks on GitHub and elsewhere. The latter will be lost in the wind.

• Do not change the Builder constructor to use brace-initialization. This is unrelated to your change. I.e. try to stick to the existing code's current style.

• In this specific case, the original code that initialized lock_file_path_ in the constructor body seems to be easier to follow, since the value is conditionally updated just after that. Performance is really not important in this constructor so I'd recommend instead to either add a comment to disable that specific cppcheck, or write a static method that computes the initialization value based on a State* value, and returns a new std::string, then call it when member-initializing lock_file_path_, e.g.: ..., lock_file_path_(ComputeLockFilePath(state_)), .... The latter is probably not worth it though.

• Do not overuse auto, in particular when defining integer values. The types of kMargin and elide_size should be size_t to ensure no compiler complains about signed vs unsigned comparisons (as on line 922). Also, replace auto result = std::string{}; with std::string result; which is smaller, simpler and 100% equivalent.

• This is a little bit more subtle, but avoid static constexpr or static const local variables if you can (i.e. just use constexpr or const when possible). As surprising as it may be, in many cases they force the allocation of values in the .text segment and force a relocation + pointer load for them in the generated code, while using non-static variables allows the compiler to perform many more optimizations. This is also true for small C string literals.

To be honest, I don't think your changes have significant value, though I am not opposed to them (after proper fixes), but @jhasse who is the official Ninja maintainer may have its own opinion on this.

Regarding stylistic large-scale changes, I think @jhasse, prefers to avoid them for the moment, as their benefits are low compared to their drawbacks.

[jhasse]

I think the lock_file_path_ optimization isn't worth it, I would just keep it as is.

The ElideMiddle change might have an impact. Do you have any numbers?

In general I would suggest to uninstall cppcheck and never use it. Use clang-tidy instead.