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.

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

Fix forward declarations & replace fwd header files #13972

Merged

Conversation

cngzhnp
Copy link
Contributor

@cngzhnp cngzhnp commented Oct 10, 2020

I had done before in #13623 and these are new fixes for other files as well. I try to use forward declaration files as much as possible. Please give feedback about if new files need to be created.

@cngzhnp cngzhnp force-pushed the cp/fix_forward_declaration_part3 branch from 781efa2 to 0de673d Compare October 11, 2020 20:42
@PhoebeHui PhoebeHui self-assigned this Oct 12, 2020
@PhoebeHui PhoebeHui added the category:code-cleanup This PR cleans up code, without fixing any existing bugs nor adding any features. label Oct 12, 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.

Just the one thing, otherwise this is so great 😄

@@ -1,16 +1,12 @@
#pragma once

#include <vcpkg/fwd/dependencies.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

namespace vckpg::Parse
{
struct ParseControlErrorInfo;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this not in vcpkg/fwd/paragraphparser.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Escaped my notice, I will add a forward declaration file for this :)

@cngzhnp cngzhnp force-pushed the cp/fix_forward_declaration_part3 branch from 0de673d to b31623f Compare October 12, 2020 19:11
@cngzhnp cngzhnp force-pushed the cp/fix_forward_declaration_part3 branch from b31623f to 294a0d4 Compare October 12, 2020 19:14
@BillyONeal
Copy link
Member

(Approval obviously conditioned on a successful build :) )

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Oct 12, 2020

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 13972 in repo microsoft/vcpkg

@BillyONeal
Copy link
Member

@cngzhnp It got your changes with out azp run : https://dev.azure.com/vcpkg/public/_build/results?buildId=43989&view=results

@cngzhnp cngzhnp force-pushed the cp/fix_forward_declaration_part3 branch from 294a0d4 to a609048 Compare October 12, 2020 19:23
@cngzhnp
Copy link
Contributor Author

cngzhnp commented Oct 12, 2020

@BillyONeal thanks, maybe not a good place to mention or ask but if the azure-pipelines bot can do format with commands here, it would be easier for us. I always forget to run the format tool before committing the changes :)

@BillyONeal
Copy link
Member

Yeah, it's something we've wanted to do; unfortunately we're stuck between a rock and a hard place here. Azure Pipelines can't write any changes back to GitHub (like the formatting changes we want), and GitHub Actions can't control Azure to create and destroy the test machines (we need bigger than the dual core boxes Actions come with).

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Oct 13, 2020

Yeah, it's something we've wanted to do; unfortunately we're stuck between a rock and a hard place here. Azure Pipelines can't write any changes back to GitHub (like the formatting changes we want), and GitHub Actions can't control Azure to create and destroy the test machines (we need bigger than the dual core boxes Actions come with).

Thanks for the reply @BillyONeal. Now, it is ready to merge I guess. (condition is checked) :)

@cngzhnp cngzhnp force-pushed the cp/fix_forward_declaration_part3 branch from a609048 to 4722aa4 Compare October 24, 2020 18:58
@cngzhnp cngzhnp force-pushed the cp/fix_forward_declaration_part3 branch from 4722aa4 to a195971 Compare October 24, 2020 19:03
@BillyONeal BillyONeal merged commit 2f731d6 into microsoft:master Oct 25, 2020
@BillyONeal
Copy link
Member

Thanks for your contribution!

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:code-cleanup This PR cleans up code, without fixing any existing bugs nor adding any features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants