Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove for/if antipattern in PreBuildInfo::PreBuildInfo. #1362

Merged
merged 2 commits into from
Mar 21, 2024
Merged

Remove for/if antipattern in PreBuildInfo::PreBuildInfo. #1362

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6de751b
Remove for/if antipattern in PreBuildInfo::PreBuildInfo.
BillyONeal Mar 7, 2024
5b97241
Merge remote-tracking branch 'origin/main' into bill-hates-for-if-ant…
BillyONeal Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 28 additions & 10 deletions include/vcpkg/base/util.h
View file
Open in desktop
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use optional here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
156 changes: 45 additions & 111 deletions src/vcpkg/commands.build.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1756,121 +1756,55 @@ 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,
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"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be on 1821

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true. But not looking to make functional changes here.

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);
}
Comment on lines +1785 to +1793
Copy link
Contributor

@Neumann-A Neumann-A Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like value_if_set_and_valid (Use lambda as validator?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be the same as optional. See my comment above.


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},
// 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::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);
// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe separate side effects from the value_if_set_and_nonempty and make it a two step process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason they're combined here is to preserve the table-like structure for the fields where that is actually possible

}

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