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

[vcpkg] Use more forward declarations rather than definitions #13623

Merged

Conversation

cngzhnp
Copy link
Contributor

@cngzhnp cngzhnp commented Sep 20, 2020

I had done before in #12836 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.

@PhoebeHui PhoebeHui added the category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) label Sep 21, 2020
@PhoebeHui PhoebeHui changed the title Fix forward declarations for a few files [vcpkg] Fix forward declarations for a few files Sep 21, 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.

Thanks for doing this @cngzhnp :)

@BillyONeal
Copy link
Member

The CI failures look legitimate

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Sep 21, 2020

Yes, I did not realize that compilation of tests is failed. Is there any batch file to run it or just run cmake on vcpkg-test?

@BillyONeal
Copy link
Member

Yes, I did not realize that compilation of tests is failed. Is there any batch file to run it or just run cmake on vcpkg-test?

Right, you can see the exact commands we run in the ymls in $/scripts/azure-pipelines.

@BillyONeal
Copy link
Member

It does indeed look like configuring CMake with tests on:

option(BUILD_TESTING "Option for enabling testing" ON)

rather than trying to use the msbuild based bootstrap would show you these failures:

"C:\PROGRA~2\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\bin\Hostx86\x86\cl.exe"  /nologo /TP -DVCPKG_USE_STD_FILESYSTEM=1 -IC:\a\1\s\toolsrc\include /DWIN32 /D_WINDOWS  /GR /EHsc /MDd /Zi /Ob0 /Od /RTC1   -FC -permissive- -utf-8 -W4 -analyze -WX -std:c++17 /showIncludes /FoCMakeFiles\vcpkg-test.dir\src\vcpkg-test\update.cpp.obj /FdCMakeFiles\vcpkg-test.dir\ /FS -c C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(21): error C2079: 'status_db' uses undefined struct 'vcpkg::StatusParagraphs'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(21): error C2440: 'initializing': cannot convert from 'std::vector<std::unique_ptr<vcpkg::StatusParagraph,std::default_delete<vcpkg::StatusParagraph>>,std::allocator<std::unique_ptr<vcpkg::StatusParagraph,std::default_delete<vcpkg::StatusParagraph>>>>' to 'int'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(21): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(26): error C2039: 'MapPortFileProvider': is not a member of 'vcpkg::PortFileProvider'
C:\a\1\s\toolsrc\include\vcpkg\update.h(15): note: see declaration of 'vcpkg::PortFileProvider'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(26): error C2065: 'MapPortFileProvider': undeclared identifier
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(26): error C2146: syntax error: missing ';' before identifier 'provider'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(26): error C3861: 'provider': identifier not found
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(28): error C2065: 'provider': undeclared identifier
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(28): error C2440: '<function-style-cast>': cannot convert from 'bool (__cdecl *)(const vcpkg::Update::OutdatedPackage &,const vcpkg::Update::OutdatedPackage &)' to 'vcpkg::SortedVector<vcpkg::Update::OutdatedPackage>'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(29): note: No constructor could take the source type, or constructor overload resolution was ambiguous
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(31): error C3536: 'pkgs': cannot be used before it is initialized
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(32): error C2109: subscript requires array or pointer type
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(33): error C2109: subscript requires array or pointer type
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(45): error C2079: 'status_db' uses undefined struct 'vcpkg::StatusParagraphs'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(45): error C2440: 'initializing': cannot convert from 'std::vector<std::unique_ptr<vcpkg::StatusParagraph,std::default_delete<vcpkg::StatusParagraph>>,std::allocator<std::unique_ptr<vcpkg::StatusParagraph,std::default_delete<vcpkg::StatusParagraph>>>>' to 'int'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(45): note: No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(50): error C2039: 'MapPortFileProvider': is not a member of 'vcpkg::PortFileProvider'
C:\a\1\s\toolsrc\include\vcpkg\update.h(15): note: see declaration of 'vcpkg::PortFileProvider'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(50): error C2065: 'MapPortFileProvider': undeclared identifier
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(50): error C2146: syntax error: missing ';' before identifier 'provider'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(50): error C3861: 'provider': identifier not found
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(52): error C2065: 'provider': undeclared identifier
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(52): error C2440: '<function-style-cast>': cannot convert from 'bool (__cdecl *)(const vcpkg::Update::OutdatedPackage &,const vcpkg::Update::OutdatedPackage &)' to 'vcpkg::SortedVector<vcpkg::Update::OutdatedPackage>'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(53): note: No constructor could take the source type, or constructor overload resolution was ambiguous
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(55): error C3536: 'pkgs': cannot be used before it is initialized
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(56): error C2109: subscript requires array or pointer type
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(57): error C2109: subscript requires array or pointer type
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(71): error C2079: 'status_db' uses undefined struct 'vcpkg::StatusParagraphs'
C:\a\1\s\toolsrc\src\vcpkg-test\update.cpp(71): error C2440: 'initializing': cannot convert from 'std::vector<std::unique_ptr<vcpkg::StatusParagraph,std::default_delete<vcpkg::StatusParagraph>>,std::allocator<std::unique_ptr<vcpkg::StatusParagraph,std::default_delete<vcpkg::StatusParagraph>>>>' to 'int'

Billy3

@cngzhnp
Copy link
Contributor Author

cngzhnp commented Sep 21, 2020

@BillyONeal Thanks, at least I could generate VS19 solution from CMakeLists.txt and saw that update.cpp was failed during compilation. It seems that it is fixed right now but let's see what CI brings to me :)

@BillyONeal
Copy link
Member

It seems like for most of these we should extract _fwd headers rather than redeclaration?

@PhoebeHui PhoebeHui added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) requires:discussion labels Sep 22, 2020
@BillyONeal
Copy link
Member

It seems like for most of these we should extract _fwd headers rather than redeclaration?

To clarify, I'm saying we should apply Nicole's feedback in the previous PR ( #12836 (comment) ) here too

@JackBoosY JackBoosY added category:code-cleanup This PR cleans up code, without fixing any existing bugs nor adding any features. and removed category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed labels Sep 22, 2020
@cngzhnp
Copy link
Contributor Author

cngzhnp commented Sep 22, 2020

@BillyONeal It is OK from my perspective. I've mentioned in PR description if new files need to be created, I can do it in this PR. Otherwise, we can close this, create new one to refer this and make changes in there. What do you think?

@BillyONeal
Copy link
Member

I would feel better if _fwds are used rather than duplicating declarations.

@cngzhnp cngzhnp force-pushed the cp/fix_forward_declaration_part_2 branch from 8983270 to d1f1c8f Compare September 26, 2020 08:55
@cngzhnp
Copy link
Contributor Author

cngzhnp commented Sep 26, 2020

Could you please review my changes again @BillyONeal @strega-nil?

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Sep 29, 2020
@dan-shaw
Copy link
Contributor

dan-shaw commented Oct 6, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@BillyONeal BillyONeal changed the title [vcpkg] Fix forward declarations for a few files [vcpkg] Use more forward declarations rather than definitions Oct 9, 2020
@BillyONeal BillyONeal merged commit 0dcc11a into microsoft:master Oct 9, 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
…oft#13623)

Co-authored-by: Billy Robert O'Neal III <bion@microsoft.com>
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. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants