[vcpkg] Fix forward declarations and missing pragma once by cngzhnp · Pull Request #12836 · microsoft/vcpkg

cngzhnp · 2020-08-09T22:54:42Z

Most of the header files does contain a forward declaration for some incomplete types. In this pull request, I tried to add them to break the dependency between header files as well.

On the other hand, buildenvironment.h file does not contain "pragma once" and fixed.

ghost · 2020-08-09T22:54:54Z

All CLA requirements met.

strega-nil · 2020-08-10T16:48:06Z

@cngzhnp Could you instead add a -fwd file for all of these, instead of just declaring the types? Also, we are not allowed to forward declare types from the standard library, so that -fwd file will have to include <filesystem>.

toolsrc/include/vcpkg/archives.h

cngzhnp · 2020-08-12T06:53:01Z

There was a failure on the pipeline but I checked the logs, assume no related with my changes. Could you please restart the pipeline and review my changes?

JackBoosY · 2020-08-12T07:25:15Z

Some of them are related:

-- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Success
-- Found Threads: TRUE  
-- Configuring done
-- Generating done
-- Build files have been written to: /Volumes/data/work/3/s/toolsrc/build.rel
[0/2] Re-checking globbed directories...
[1/106] Building CXX object CMakeFiles/vcpkg-test.dir/src/vcpkg-test/chrono.cpp.o
[2/106] Building CXX object CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o
FAILED: CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o 
/Library/Developer/CommandLineTools/usr/bin/g++  -DVCPKG_USE_STD_FILESYSTEM=1 -I../include -O3 -DNDEBUG -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk   -std=c++17 -MD -MT CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o -MF CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o.d -o CMakeFiles/vcpkg.dir/src/vcpkg.cpp.o -c ../src/vcpkg.cpp
In file included from ../src/vcpkg.cpp:11:
In file included from ../include/vcpkg/commands.contact.h:3:
../include/vcpkg/commands.interface.h:9:12: error: forward declaration of struct cannot have a nested name specifier
    struct Files::Filesystem;
           ^~~~~~~
1 error generated.
[3/106] Building CXX object CMakeFiles/vcpkg-test.dir/src/vcpkg-test/commands.build.cpp.o
FAILED: CMakeFiles/vcpkg-test.dir/src/vcpkg-test/commands.build.cpp.o 
/Library/Developer/CommandLineTools/usr/bin/g++  -DVCPKG_USE_STD_FILESYSTEM=1 -I../include -O3 -DNDEBUG -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk   -std=c++17 -MD -MT CMakeFiles/vcpkg-test.dir/src/vcpkg-test/commands.build.cpp.o -MF CMakeFiles/vcpkg-test.dir/src/vcpkg-test/commands.build.cpp.o.d -o CMakeFiles/vcpkg-test.dir/src/vcpkg-test/commands.build.cpp.o -c ../src/vcpkg-test/commands.build.cpp
In file included from ../src/vcpkg-test/commands.build.cpp:5:
In file included from ../include/vcpkg/commands.h:3:
In file included from ../include/vcpkg/build.h:9:
In file included from ../include/vcpkg/commands.integrate.h:3:
../include/vcpkg/commands.interface.h:9:12: error: forward declaration of struct cannot have a nested name specifier
    struct Files::Filesystem;
           ^~~~~~~
1 error generated.
[4/106] Building CXX object CMakeFiles/vcpkg-test.dir/src/vcpkg-test/binarycaching.cpp.o
FAILED: CMakeFiles/vcpkg-test.dir/src/vcpkg-test/binarycaching.cpp.o 
/Library/Developer/CommandLineTools/usr/bin/g++  -DVCPKG_USE_STD_FILESYSTEM=1 -I../include -O3 -DNDEBUG -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk   -std=c++17 -MD -MT CMakeFiles/vcpkg-test.dir/src/vcpkg-test/binarycaching.cpp.o -MF CMakeFiles/vcpkg-test.dir/src/vcpkg-test/binarycaching.cpp.o.d -o CMakeFiles/vcpkg-test.dir/src/vcpkg-test/binarycaching.cpp.o -c ../src/vcpkg-test/binarycaching.cpp
In file included from ../src/vcpkg-test/binarycaching.cpp:6:
In file included from ../include/vcpkg/binarycaching.private.h:3:
In file included from ../include/vcpkg/dependencies.h:6:
In file included from ../include/vcpkg/build.h:9:
In file included from ../include/vcpkg/commands.integrate.h:3:
../include/vcpkg/commands.interface.h:9:12: error: forward declaration of struct cannot have a nested name specifier
    struct Files::Filesystem;
           ^~~~~~~
1 error generated.
[5/106] Building CXX object CMakeFiles/vcpkg-test.dir/src/vcpkg-test/binaryconfigparser.cpp.o
[6/106] Building CXX object CMakeFiles/vcpkg-test.dir/src/vcpkg-test/arguments.cpp.o
[7/106] Building CXX object CMakeFiles/vcpkg-test.dir/src/vcpkg-test/commands.cpp.o
FAILED: CMakeFiles/vcpkg-test.dir/src/vcpkg-test/commands.cpp.o

strega-nil · 2020-08-12T16:36:00Z

/Library/Developer/CommandLineTools/usr/bin/g++  -DVCPKG_USE_STD_FILESYSTEM=1 -I../include -O3 -DNDEBUG -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk   -include /Volumes/data/work/3/s/toolsrc/include/pch.h -std=c++17 -MD -MT CMakeFiles/vcpkglib.dir/src/vcpkg/commands.xvsinstances.cpp.o -MF CMakeFiles/vcpkglib.dir/src/vcpkg/commands.xvsinstances.cpp.o.d -o CMakeFiles/vcpkglib.dir/src/vcpkg/commands.xvsinstances.cpp.o -c ../src/vcpkg/commands.xvsinstances.cpp
../src/vcpkg/commands.xvsinstances.cpp:30:9: error: use of undeclared identifier 'Util'
        Util::unused(args, paths);

Could you remove Util::unused and switch to (void)?

cngzhnp · 2020-08-12T18:35:25Z

/Library/Developer/CommandLineTools/usr/bin/g++  -DVCPKG_USE_STD_FILESYSTEM=1 -I../include -O3 -DNDEBUG -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk   -include /Volumes/data/work/3/s/toolsrc/include/pch.h -std=c++17 -MD -MT CMakeFiles/vcpkglib.dir/src/vcpkg/commands.xvsinstances.cpp.o -MF CMakeFiles/vcpkglib.dir/src/vcpkg/commands.xvsinstances.cpp.o.d -o CMakeFiles/vcpkglib.dir/src/vcpkg/commands.xvsinstances.cpp.o -c ../src/vcpkg/commands.xvsinstances.cpp
../src/vcpkg/commands.xvsinstances.cpp:30:9: error: use of undeclared identifier 'Util'
        Util::unused(args, paths);

Could you remove Util::unused and switch to (void)?

@strega-nil Done.

cngzhnp · 2020-08-15T11:12:33Z

I do not understand that why "extern const CommandStructure COMMAND_STRUCTURE;" is added on some header files (maybe pre-work for next features) but it makes extra dependent header files to vcpkgcmdarguments.h.

JackBoosY · 2020-08-17T06:06:26Z

@strega-nil Ping for review.

BillyONeal · 2020-08-17T16:15:02Z

toolsrc/src/vcpkg/commands.xvsinstances.cpp

        Checks::exit_success(VCPKG_LINE_INFO);
 #else
-        Util::unused(args, paths);
+        (void)(args, paths);


It would be nice to see complete elimination of Util::unused but that's a separate change.

That was requested from @strega-nil. Not related with context but added.

Yes, I mean do what she asked but for all callers of Util::unused rather than just here.

strega-nil · 2020-08-17T16:26:46Z

I'd like to see something like cngzhnp#1 used instead, since I don't like copy-pasting things that we can get the compiler to copy-paste for us.

BillyONeal · 2020-08-18T17:55:51Z

@strega-nil Can you clarify: are you saying this PR should rejected for now? (If so, can you mark it 'waiting'/'request changes'?)

strega-nil

Could we do cngzhnp#1 instead?

cngzhnp · 2020-08-19T13:13:48Z

@strega-nil From my point of view, LGTM.

BillyONeal · 2020-08-19T22:32:53Z

@cngzhnp Thanks for your contribution! I'm closing this in favor of #12985

PhoebeHui assigned PhoebeHui and JackBoosY Aug 10, 2020

JackBoosY requested review from BillyONeal and strega-nil August 10, 2020 03:24

PhoebeHui changed the title ~~Fix forward declarations and missing pragma once~~ [vcpkg] Fix forward declarations and missing pragma once Aug 10, 2020

BillyONeal requested changes Aug 10, 2020

View reviewed changes

toolsrc/include/vcpkg/archives.h Outdated Show resolved Hide resolved

JackBoosY added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) requires:author-response labels Aug 11, 2020

cngzhnp force-pushed the cngzhnp/fix_forward_declarations branch 2 times, most recently from e3f28d0 to eff34ae Compare August 11, 2020 21:45

cngzhnp requested a review from BillyONeal August 12, 2020 06:50

cngzhnp force-pushed the cngzhnp/fix_forward_declarations branch from eff34ae to 8eeed12 Compare August 12, 2020 07:35

cngzhnp force-pushed the cngzhnp/fix_forward_declarations branch from 8eeed12 to 63f3bb7 Compare August 12, 2020 18:33

cngzhnp force-pushed the cngzhnp/fix_forward_declarations branch from 63f3bb7 to e1264a4 Compare August 15, 2020 11:10

Fix forward declarations and missing pragma once

24878f7

cngzhnp force-pushed the cngzhnp/fix_forward_declarations branch from e1264a4 to 24878f7 Compare August 15, 2020 11:15

JackBoosY added requires:discussion and removed requires:author-response labels Aug 17, 2020

BillyONeal approved these changes Aug 17, 2020

View reviewed changes

JackBoosY added requires:author-response and removed requires:discussion labels Aug 18, 2020

strega-nil suggested changes Aug 18, 2020

View reviewed changes

strega-nil mentioned this pull request Aug 18, 2020

[vcpkg] Add Forward Declarations (originally by @cngzhnp) #12985

Merged

BillyONeal closed this Aug 19, 2020

cngzhnp mentioned this pull request Sep 20, 2020

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

Merged

Conversation

cngzhnp commented Aug 9, 2020

Uh oh!

ghost commented Aug 9, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil commented Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cngzhnp commented Aug 12, 2020

Uh oh!

JackBoosY commented Aug 12, 2020

Uh oh!

strega-nil commented Aug 12, 2020

Uh oh!

cngzhnp commented Aug 12, 2020

Uh oh!

cngzhnp commented Aug 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackBoosY commented Aug 17, 2020

Uh oh!

BillyONeal Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

cngzhnp Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

BillyONeal Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

strega-nil commented Aug 17, 2020

Uh oh!

BillyONeal commented Aug 18, 2020

Uh oh!

strega-nil left a comment

Choose a reason for hiding this comment

Uh oh!

cngzhnp commented Aug 19, 2020

Uh oh!

BillyONeal commented Aug 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ghost commented Aug 9, 2020 •

edited by ghost

Loading

strega-nil commented Aug 10, 2020 •

edited

Loading

cngzhnp commented Aug 15, 2020 •

edited

Loading