Skip to content

[vcpkg] Fix warning (as error) when building vcpgk/toolsrc with clang++ 10 or g++ 9.3 (see #15148)#15149

Merged
BillyONeal merged 6 commits intomicrosoft:masterfrom
klalumiere:fix-15148
Dec 17, 2020
Merged

[vcpkg] Fix warning (as error) when building vcpgk/toolsrc with clang++ 10 or g++ 9.3 (see #15148)#15149
BillyONeal merged 6 commits intomicrosoft:masterfrom
klalumiere:fix-15148

Conversation

@klalumiere
Copy link
Copy Markdown
Contributor

@klalumiere klalumiere commented Dec 16, 2020

Describe the pull request

It now compiles (and then the tests succeed) with both clang++ 10 and g++ 9.3.

  • Which triplets are supported/not supported? Have you updated the CI baseline? Does not apply.

  • Does your PR follow the maintainer guide? Yes

@NancyLi1013 NancyLi1013 added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) requires:discussion labels Dec 16, 2020
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 16, 2020

Can you fix what the thing is warning about rather than silencing the warning?

@klalumiere
Copy link
Copy Markdown
Contributor Author

klalumiere commented Dec 16, 2020

Can you fix what the thing is warning about rather than silencing the warning?

Yes I can. The reason I didn't do it to begin with is that the warning [1] is a little bit more subtle than the other two. In particular, there's (at least) two ways to fix the issue highlighted by it:

  1. Declare PathsPortFileProvider as final.
  2. Add a virtual destructor to PortFileProvider.

In principle, the way we choose to fix it should ultimately depends on the intent of the author of the code. In practice, it probably won't matter a lot. I thought the warning was pedantic (which is what we asked the compiler), so I choose the third option to silence it locally.

I'll add a virtual destructor for now since this fix is "local" (if I choose the alternative, i.e. to make PathsPortFileProvider final, I should probably also make MapPortFileProvider final). I'm happy to discuss it or to go the other way if you think it's better 🙂 .

[1]

[80/102] Building CXX object CMakeFiles/vcpkglib.dir/src/vcpkg/portfileprovider.cpp.o
FAILED: CMakeFiles/vcpkglib.dir/src/vcpkg/portfileprovider.cpp.o 
/usr/bin/clang++  -DVCPKG_USE_STD_FILESYSTEM=1 -I../include -g   -Wall -Wextra -Wpedantic -Wno-unknown-pragmas -Wno-missing-field-initializers -Wno-redundant-move -Wmissing-prototypes -Werror -include /home/klalumiere/Projects/vcpkg-klalumiere/toolsrc/include/pch.h -pthread -std=c++17 -MD -MT CMakeFiles/vcpkglib.dir/src/vcpkg/portfileprovider.cpp.o -MF CMakeFiles/vcpkglib.dir/src/vcpkg/portfileprovider.cpp.o.d -o CMakeFiles/vcpkglib.dir/src/vcpkg/portfileprovider.cpp.o -c ../src/vcpkg/portfileprovider.cpp
In file included from <built-in>:1:
In file included from /home/klalumiere/Projects/vcpkg-klalumiere/toolsrc/include/pch.h:5:
In file included from ../include/vcpkg/base/files.h:14:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/filesystem:37:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/fs_path.h:37:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/locale:43:
In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/locale_conv.h:41:
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:81:2: error: delete called on non-final 'vcpkg::PortFileProvider::PathsPortFileProvider' that has virtual functions but non-virtual destructor [-Werror,-Wdelete-non-abstract-non-virtual-dtor]
        delete __ptr;
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:292:4: note: in instantiation of member function 'std::default_delete<vcpkg::PortFileProvider::PathsPortFileProvider>::operator()' requested here
          get_deleter()(std::move(__ptr));
          ^
../src/vcpkg/portfileprovider.cpp:299:13: note: in instantiation of member function 'std::unique_ptr<vcpkg::PortFileProvider::PathsPortFileProvider, std::default_delete<vcpkg::PortFileProvider::PathsPortFileProvider> >::~unique_ptr' requested here
            BaselineProviderImpl(const VcpkgPaths& paths) : paths(paths) { }
            ^
1 error generated.

@klalumiere
Copy link
Copy Markdown
Contributor Author

klalumiere commented Dec 16, 2020

I recompiled and re-ran the test with both clang++ 10 and g++ 9.3. Everything is green ✔️.

@BillyONeal BillyONeal merged commit a268c5a into microsoft:master Dec 17, 2020
@BillyONeal
Copy link
Copy Markdown
Member

BillyONeal commented Dec 17, 2020

Thanks for squashing those warnings!

@klalumiere klalumiere deleted the fix-15148 branch December 17, 2020 15:44
ryukw7 pushed a commit to ryukw7/vcpkg that referenced this pull request Dec 24, 2020
@klalumiere
[vcpkg] Fix warning (as error) when building vcpgk/toolsrc with clang…
cfcb59f 
…++ 10 or g++ 9.3 (see microsoft#15148) (microsoft#15149)
strega-nil pushed a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
@klalumiere
[vcpkg] Fix warning (as error) when building vcpgk/toolsrc with clang…
deebf7f 
…++ 10 or g++ 9.3 (see microsoft#15148) (microsoft#15149)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@NancyLi1013 NancyLi1013

Labels

category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[vcpkg] Warning (as error) when building vcpgk/toolsrc with clang++ 10 or g++ 9.3

4 participants

@klalumiere @BillyONeal @vicroms @NancyLi1013