Remove for/if antipattern in PreBuildInfo::PreBuildInfo. #1362
Conversation
The for loop starting on 1798 looked up each entry from its associated table, then had an individual if for each entry in the table. As a result, it wasn't truly being data driven since there was an if for each entry in the table. Moreover, the table had a circular dependency with the code since LOAD_VCVARS_ENV had to come after CHAINLOAD_TOOLCHAIN_FILE. I assume this was done to deduplicate the work of figuring out if the value was set since that is the common block at the top of the loop. Instead of the for/if antipattern I solved that problem by extracting functions. The function Util::assign_if_set_and_nonempty preserves the table-like behavior for those values which are simple assignments as much as possible. The function Util::value_if_set_and_nonempty is used for the settings that needed additional work.
| Util::assign_if_set_and_nonempty(external_toolchain_file, cmakevars, CMakeVariableChainloadToolchainFile); | ||
| if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableBuildType)) | ||
| { | ||
| if (Strings::case_insensitive_ascii_equals(*value, "debug")) |
There was a problem hiding this comment.
This used to be on 1821
There was a problem hiding this comment.
vcpkg does not really support build type being set to debug only. It will basically error in all the places, post build checks being one of them.
There was a problem hiding this comment.
That's true. But not looking to make functional changes here.
|
Will create a merge conflict for #797 which still is not merged for some reason..... |
|
hmm there is already a merge conflict..... I am kind of annoyed that it has not been merged before that happening..... |
| if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableBuildType)) | ||
| { | ||
| if (Strings::case_insensitive_ascii_equals(*value, "debug")) | ||
| build_type = ConfigurationType::Debug; | ||
| else if (Strings::case_insensitive_ascii_equals(*value, "release")) | ||
| build_type = ConfigurationType::Release; | ||
| else | ||
| Checks::msg_exit_with_message(VCPKG_LINE_INFO, msgUnknownSettingForBuildType, msg::option = *value); | ||
| } |
There was a problem hiding this comment.
this looks like value_if_set_and_valid (Use lambda as validator?)
There was a problem hiding this comment.
This would be the same as optional. See my comment above.
src/vcpkg/commands.build.cpp
Outdated
| load_vcvars_env = !external_toolchain_file.has_value(); | ||
| if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableLoadVcvarsEnv)) | ||
| { | ||
| load_vcvars_env = from_cmake_bool(*value, CMakeVariableLoadVcvarsEnv).value_or_exit(VCPKG_LINE_INFO); | ||
| } | ||
|
|
||
| if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableDisableCompilerTracking)) | ||
| { | ||
| disable_compiler_tracking = | ||
| from_cmake_bool(*value, CMakeVariableDisableCompilerTracking).value_or_exit(VCPKG_LINE_INFO); | ||
| } | ||
|
|
||
| if (Util::value_if_set_and_nonempty(cmakevars, CMakeVariableXBoxConsoleTarget) != nullptr) | ||
| { | ||
| target_is_xbox = true; | ||
| } | ||
|
|
||
| Util::assign_if_set_and_nonempty(gamedk_latest_path, cmakevars, CMakeVariableZVcpkgGameDKLatest); |
There was a problem hiding this comment.
Maybe separate side effects from the value_if_set_and_nonempty and make it a two step process?
There was a problem hiding this comment.
The reason they're combined here is to preserve the table-like structure for the fields where that is actually possible
| } | ||
|
|
||
| template<class Map, class Key> | ||
| void value_or_default(const Map& map, Key&& key, typename Map::mapped_type&& default_value) = delete; | ||
| inline const std::string* value_if_set_and_nonempty(const std::unordered_map<std::string, std::string>& haystack, |
There was a problem hiding this comment.
Why not use optional here?
There was a problem hiding this comment.
Optional would imply copying the content, and we don't need to copy the content. The string is already in the unordered_map, so we can just use a pointer to it.
Optional is only necessary in cases where the thing must own the actual data.
| if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableBuildType)) | ||
| { | ||
| if (Strings::case_insensitive_ascii_equals(*value, "debug")) | ||
| build_type = ConfigurationType::Debug; | ||
| else if (Strings::case_insensitive_ascii_equals(*value, "release")) | ||
| build_type = ConfigurationType::Release; | ||
| else | ||
| Checks::msg_exit_with_message(VCPKG_LINE_INFO, msgUnknownSettingForBuildType, msg::option = *value); | ||
| } |
There was a problem hiding this comment.
This would be the same as optional. See my comment above.
| { | ||
| const auto it = map.find(static_cast<Key&&>(key)); | ||
| if (it == map.end()) | ||
| auto it = haystack.find(needle.to_string()); |
There was a problem hiding this comment.
The need to allocate strings for every lookup here seems very unfortunate -- is it an extensive change to push transparency up into the data structures? (replace unordered_map with map, etc).
There was a problem hiding this comment.
Because I don't have a good understanding of the performance consequences of a change like that, and it isn't on the path for any of the customer facing improvements I'm trying to ship, I'm not considering doing that right now.
…i-pattern # Conflicts: # src/vcpkg/commands.build.cpp
The for loop starting on 1798 looked up each entry from its associated table, then had an individual if for each entry in the table. As a result, it wasn't truly being data driven since there was an if for each entry in the table. Moreover, the table had a circular dependency with the code since LOAD_VCVARS_ENV had to come after CHAINLOAD_TOOLCHAIN_FILE. I assume this was done to deduplicate the work of figuring out if the value was set since that is the common block at the top of the loop. Instead of the for/if antipattern I solved that problem by extracting functions.
The function Util::assign_if_set_and_nonempty preserves the table-like behavior for those values which are simple assignments as much as possible. The function Util::value_if_set_and_nonempty is used for the settings that needed additional work.