Skip to content

Conversation

BillyONeal
Copy link
Member

…, so that we need sign only one binary.

@BillyONeal BillyONeal added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed info:internal labels Sep 9, 2020
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

I would prefer to see the ifdef solution rather than the REMOVE_ITEM solution, because I think it's cleaner.

memset(&startup_info, 0, sizeof(STARTUPINFOW));
startup_info.cb = sizeof(STARTUPINFOW);
startup_info.dwFlags = STARTF_USESHOWWINDOW;
startup_info.wShowWindow = SW_HIDE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we always want these flags?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we do but the bits could be renamed to be clearer, will fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does my rename resolve your concern?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very much, thanks!

Commands::Version::VersionCommand version{};
}
template<class CommandListT, size_t ExpectedCount>
void check_all_commands(const CommandListT& actual_commands, const char* const (&expected_commands)[ExpectedCount])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion: take std::initializer_list<StringLiteral> as the second argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that's an improvement: initializer_list exists to be the thing that controls constructor overload resolution; using it for any other purpose just encourages use after free bugs when people treat it like a container instead of a pointer to a temporary array.

memset(&startup_info, 0, sizeof(STARTUPINFOW));
startup_info.cb = sizeof(STARTUPINFOW);
startup_info.dwFlags = STARTF_USESHOWWINDOW;
startup_info.wShowWindow = SW_HIDE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Very much, thanks!

@BillyONeal BillyONeal merged commit 6b97dbf into microsoft:master Sep 11, 2020
@BillyONeal BillyONeal deleted the burninate_metadata_uploader branch September 11, 2020 20:52
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants