Skip to content

Commit

Permalink
Remove for/if antipattern in PreBuildInfo::PreBuildInfo. (#1362)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@BillyONeal
BillyONeal authored Mar 21, 2024
1 parent 2330565 commit 086859d
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 126 deletions.
38 changes: 28 additions & 10 deletions include/vcpkg/base/util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,22 +132,40 @@ namespace vcpkg::Util
return it->second;
}

template<class Map, class Key>
const typename Map::mapped_type& value_or_default(const Map& map,
Key&& key,
const typename Map::mapped_type& default_value)
inline void assign_if_set_and_nonempty(std::string& target,
const std::unordered_map<std::string, std::string>& haystack,
StringLiteral needle)
{
const auto it = map.find(static_cast<Key&&>(key));
if (it == map.end())
auto it = haystack.find(needle.to_string());
if (it != haystack.end() && !it->second.empty())
{
return default_value;
target = it->second;
}
}

return it->second;
template<class T>
void assign_if_set_and_nonempty(Optional<T>& target,
const std::unordered_map<std::string, std::string>& haystack,
StringLiteral needle)
{
auto it = haystack.find(needle.to_string());
if (it != haystack.end() && !it->second.empty())
{
target.emplace(it->second);
}
}

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,
StringLiteral needle)
{
auto it = haystack.find(needle.to_string());
if (it != haystack.end() && !it->second.empty())
{
return &it->second;
}

return nullptr;
}

template<class Map, class Key>
Optional<const typename Map::mapped_type&> lookup_value(const Map& map, Key&& key)
Expand Down
165 changes: 49 additions & 116 deletions src/vcpkg/commands.build.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1775,126 +1775,59 @@ namespace vcpkg
const std::unordered_map<std::string, std::string>& cmakevars)
: triplet(triplet), m_paths(paths)
{
enum class VcpkgTripletVar
{
TARGET_ARCHITECTURE = 0,
CMAKE_SYSTEM_NAME,
CMAKE_SYSTEM_VERSION,
PLATFORM_TOOLSET,
PLATFORM_TOOLSET_VERSION,
VISUAL_STUDIO_PATH,
CHAINLOAD_TOOLCHAIN_FILE,
BUILD_TYPE,
ENV_PASSTHROUGH,
ENV_PASSTHROUGH_UNTRACKED,
PUBLIC_ABI_OVERRIDE,
LOAD_VCVARS_ENV,
DISABLE_COMPILER_TRACKING,
HASH_ADDITIONAL_FILES,
XBOX_CONSOLE_TARGET,
Z_VCPKG_GameDKLatest
};
Util::assign_if_set_and_nonempty(target_architecture, cmakevars, CMakeVariableTargetArchitecture);
Util::assign_if_set_and_nonempty(cmake_system_name, cmakevars, CMakeVariableCMakeSystemName);
Util::assign_if_set_and_nonempty(cmake_system_version, cmakevars, CMakeVariableCMakeSystemVersion);
Util::assign_if_set_and_nonempty(platform_toolset, cmakevars, CMakeVariablePlatformToolset);
Util::assign_if_set_and_nonempty(platform_toolset_version, cmakevars, CMakeVariablePlatformToolsetVersion);
Util::assign_if_set_and_nonempty(visual_studio_path, cmakevars, CMakeVariableVisualStudioPath);
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"))
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);
}

static constexpr std::pair<StringLiteral, VcpkgTripletVar> VCPKG_OPTIONS[] = {
{CMakeVariableTargetArchitecture, VcpkgTripletVar::TARGET_ARCHITECTURE},
{CMakeVariableCMakeSystemName, VcpkgTripletVar::CMAKE_SYSTEM_NAME},
{CMakeVariableCMakeSystemVersion, VcpkgTripletVar::CMAKE_SYSTEM_VERSION},
{CMakeVariablePlatformToolset, VcpkgTripletVar::PLATFORM_TOOLSET},
{CMakeVariablePlatformToolsetVersion, VcpkgTripletVar::PLATFORM_TOOLSET_VERSION},
{CMakeVariableVisualStudioPath, VcpkgTripletVar::VISUAL_STUDIO_PATH},
{CMakeVariableChainloadToolchainFile, VcpkgTripletVar::CHAINLOAD_TOOLCHAIN_FILE},
{CMakeVariableBuildType, VcpkgTripletVar::BUILD_TYPE},
{CMakeVariableEnvPassthrough, VcpkgTripletVar::ENV_PASSTHROUGH},
{CMakeVariableEnvPassthroughUntracked, VcpkgTripletVar::ENV_PASSTHROUGH_UNTRACKED},
{CMakeVariablePublicAbiOverride, VcpkgTripletVar::PUBLIC_ABI_OVERRIDE},
{CMakeVariableHashAdditionalFiles, VcpkgTripletVar::HASH_ADDITIONAL_FILES},
// Note: this value must come after CMakeVariableChainloadToolchainFile because its default depends upon it.
{CMakeVariableLoadVcvarsEnv, VcpkgTripletVar::LOAD_VCVARS_ENV},
{CMakeVariableDisableCompilerTracking, VcpkgTripletVar::DISABLE_COMPILER_TRACKING},
{CMakeVariableXBoxConsoleTarget, VcpkgTripletVar::XBOX_CONSOLE_TARGET},
{CMakeVariableZVcpkgGameDKLatest, VcpkgTripletVar::Z_VCPKG_GameDKLatest},
};
if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableEnvPassthrough))
{
passthrough_env_vars_tracked = Strings::split(*value, ';');
passthrough_env_vars = passthrough_env_vars_tracked;
}

const std::string empty;
for (auto&& kv : VCPKG_OPTIONS)
// Note that this must come after CMakeVariableEnvPassthrough since the leading values come from there
if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableEnvPassthroughUntracked))
{
const auto& variable_value = Util::value_or_default(cmakevars, kv.first.to_string(), empty);
switch (kv.second)
{
case VcpkgTripletVar::TARGET_ARCHITECTURE: target_architecture = variable_value; break;
case VcpkgTripletVar::CMAKE_SYSTEM_NAME: cmake_system_name = variable_value; break;
case VcpkgTripletVar::CMAKE_SYSTEM_VERSION: cmake_system_version = variable_value; break;
case VcpkgTripletVar::PLATFORM_TOOLSET:
platform_toolset = variable_value.empty() ? nullopt : Optional<std::string>{variable_value};
break;
case VcpkgTripletVar::PLATFORM_TOOLSET_VERSION:
platform_toolset_version = variable_value.empty() ? nullopt : Optional<std::string>{variable_value};
break;
case VcpkgTripletVar::VISUAL_STUDIO_PATH:
visual_studio_path = variable_value.empty() ? nullopt : Optional<Path>{variable_value};
break;
case VcpkgTripletVar::CHAINLOAD_TOOLCHAIN_FILE:
external_toolchain_file = variable_value.empty() ? nullopt : Optional<std::string>{variable_value};
break;
case VcpkgTripletVar::BUILD_TYPE:
if (variable_value.empty())
build_type = nullopt;
else if (Strings::case_insensitive_ascii_equals(variable_value, "debug"))
build_type = ConfigurationType::Debug;
else if (Strings::case_insensitive_ascii_equals(variable_value, "release"))
build_type = ConfigurationType::Release;
else
Checks::msg_exit_with_message(
VCPKG_LINE_INFO, msgUnknownSettingForBuildType, msg::option = variable_value);
break;
case VcpkgTripletVar::ENV_PASSTHROUGH:
passthrough_env_vars_tracked = Strings::split(variable_value, ';');
Util::Vectors::append(&passthrough_env_vars, passthrough_env_vars_tracked);
break;
case VcpkgTripletVar::ENV_PASSTHROUGH_UNTRACKED:
Util::Vectors::append(&passthrough_env_vars, Strings::split(variable_value, ';'));
break;
case VcpkgTripletVar::PUBLIC_ABI_OVERRIDE:
public_abi_override = variable_value.empty() ? nullopt : Optional<std::string>{variable_value};
break;
case VcpkgTripletVar::HASH_ADDITIONAL_FILES:
hash_additional_files = Strings::split(variable_value, ';');
break;
case VcpkgTripletVar::LOAD_VCVARS_ENV:
if (variable_value.empty())
{
load_vcvars_env = !external_toolchain_file.has_value();
}
else
{
load_vcvars_env = from_cmake_bool(variable_value, kv.first).value_or_exit(VCPKG_LINE_INFO);
}
break;
case VcpkgTripletVar::DISABLE_COMPILER_TRACKING:
if (variable_value.empty())
{
disable_compiler_tracking = false;
}
else
{
disable_compiler_tracking =
from_cmake_bool(variable_value, kv.first).value_or_exit(VCPKG_LINE_INFO);
}
break;
case VcpkgTripletVar::XBOX_CONSOLE_TARGET:
if (!variable_value.empty())
{
target_is_xbox = true;
}
break;
case VcpkgTripletVar::Z_VCPKG_GameDKLatest:
if (!variable_value.empty())
{
gamedk_latest_path.emplace(variable_value);
}
break;
}
Util::Vectors::append(&passthrough_env_vars, Strings::split(*value, ';'));
}

Util::assign_if_set_and_nonempty(public_abi_override, cmakevars, CMakeVariablePublicAbiOverride);
if (auto value = Util::value_if_set_and_nonempty(cmakevars, CMakeVariableHashAdditionalFiles))
{
hash_additional_files = Strings::split(*value, ';');
}
// Note that this value must come after CMakeVariableChainloadToolchainFile because its default depends upon it
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);
}

ExtendedBuildResult::ExtendedBuildResult(BuildResult code) : code(code) { }
Expand Down

0 comments on commit 086859d

Please sign in to comment.