[build] Optimizations and Localization #555
Conversation
# Conflicts: # locales/messages.en.json # locales/messages.json # src/vcpkg/build.cpp
|
@BillyONeal I applied your suggestions. Can you review again? |
| std::map<BuildPolicy, bool> policies; | ||
| for (auto policy : ALL_POLICIES) | ||
| std::unordered_map<BuildPolicy, bool> policies; | ||
| for (const auto& policy : ALL_POLICIES) |
There was a problem hiding this comment.
If I'm not mistaken, this looks like it's being persisted to a file that we ultimately hash, so we needed the determinism std::map was giving us?
I'm not certain.
In general, I think swapping from map to unordered_map needs to prove that all places the map is being iterated don't care about order.
There was a problem hiding this comment.
build_info.policies is mainly used by post build lint. There is no other reference, so I don't think it's used for hashing. Just do a find all references.
| @@ -287,7 +345,7 @@ namespace vcpkg::Build | |||
| { | |||
| msg::print(Color::warning, warnings); | |||
| } | |||
| msg::print(Color::error, Build::create_error_message(result, spec)); | |||
| msg::println_error(Build::create_error_message(result, spec)); | |||
There was a problem hiding this comment.
| msg::println_error(Build::create_error_message(result, spec)); | |
| msg::println(Build::create_error_message(result, spec)); |
create_error_message already adds the error: if I am reading this correctly. Probably occurs elsewhere.
There was a problem hiding this comment.
Looking at Build::create_error_message, neither the function itself nor the corresponding message msgBuildingPackageFailed adds error: . I removed the prefix and added it to the calling code via println_error. Again, just use find all references.
There was a problem hiding this comment.
Looking at Build::create_error_message, neither the function itself nor the corresponding message msgBuildingPackageFailed adds error: .
Ah, I see, today it adds the error prefix, but you changed it so that it does not.
Again, just use find all references.
There's no "find all references" from the code review interface.
| { | ||
| auto it = fields->find(fieldname); | ||
| auto it = fields->find(fieldname.to_string()); |
There was a problem hiding this comment.
This is now safe but I think whether it's a perf win is questionable, since a lot of the time fieldname is already going to have been a std::string. However, I think the interface change is an improvement so I'm OK with it. Engaging C++17 transparent map comparisons can be later.
There was a problem hiding this comment.
This is now safe but I think whether it's a perf win is questionable, since a lot of the time
fieldnameis already going to have been astd::string.
Even if it was a win, anybody would notice because there are other things that are much slower. I'm already working on a PR to speed up post build lint using concurrency for some checks.
|
@BillyONeal Can you please take another look again? |
|
Thanks :) |
static const std::stringtostatic constexpr StringLiteralconst std::string&toStringViewbuild.cppstd::unordered_mapinstead ofstd::mapNote: Depends on #553