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

Inline defaulted functions #1280

Merged
merged 7 commits into from Dec 20, 2023
Merged

Conversation

Thomas1664
Copy link
Contributor

@Thomas1664 Thomas1664 commented Nov 17, 2023

In the case of out of line defaulted move constructors this prevented some types from being nothrow movable.
Please also note that associative containers aren't nothrow movable on MSVC. One has to explicitly mark default move constructors as noexcept for types containing associative containers to make them nothrow movable.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This is reverting an explicit design choice to favor build throughput by not needing the definition of these functions spammed out into every object file in places where performance does not matter.

Do you have data showing this is meaningful for customers?

include/vcpkg/platform-expression.h Show resolved Hide resolved
include/vcpkg/platform-expression.h Outdated Show resolved Hide resolved
include/vcpkg/vcpkgpaths.h Outdated Show resolved Hide resolved
include/vcpkg/xunitwriter.h Outdated Show resolved Hide resolved
@Thomas1664
Copy link
Contributor Author

Thomas1664 commented Nov 21, 2023

This is reverting an explicit design choice to favor build throughput by not needing the definition of these functions spammed out into every object file in places where performance does not matter.

Do you have data showing this is meaningful for customers?

The main goal of this PR is to avoid not marking move constructors as noexcept in the case of out of line defaulted move constructors. I highly doupt that inlining vs not inlining makes a difference at compile time or run time.

Edit:

Furthermore, implicitly declared move constructors are always inlined: https://en.cppreference.com/w/cpp/language/move_constructor Following your design, you'd have to manually default all (move) constructors out of line.

@BillyONeal
Copy link
Member

@ras0219-msft @data-queue @vicroms @JavierMatosD and I discussed this today and we are happy with this so long as defaulting in the header does not require exposing additional dependencies in headers (e.g. in pimpl patterns like this:

// header.hpp

#include <memory>

struct impl;

struct ptr {
    std::unique_ptr<impl> instance;
    // ~ptr() = default; // error: impl is incomplete, how do I ~impl?
    ~ptr();
};

)

@BillyONeal BillyONeal merged commit 51d7f89 into microsoft:main Dec 20, 2023
5 checks passed
@BillyONeal
Copy link
Member

Thanks :)

@Thomas1664 Thomas1664 deleted the inline-default branch December 20, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants