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

Keep defaulted destructors inline #2255

Merged

Conversation

DanielaE
Copy link
Contributor

applies to exception classes in case of msvc only

@vitaut
Copy link
Contributor

vitaut commented Apr 27, 2021

Thanks for the PR but could you elaborate why do this change?

@DanielaE
Copy link
Contributor Author

Otherwise I see this

fmt-m-pp.lib(fmt-m-pp.obj) : error LNK2005: "public: virtual __cdecl fmt::v7::format_error::~format_error(void)" (??1format_error@v7@fmt@@UEAA@XZ::<!fmt>) already defined in check.obj
check-m-pp.exe : fatal error LNK1169: one or more multiply defined symbols found

in my module compile check as soon as I add an example with a local custom formatter like this:

enum class check_color {red, green, blue};

namespace fmt {
template<> struct formatter<check_color> : formatter<string_view> {
// parse is inherited
    template <typename FormatContext>
    auto format(check_color c, FormatContext& ctx) {
        string_view name = "?";
        switch (c) {
            case check_color::red:   name = "red"; break;
            case check_color::green: name = "green"; break;
            case check_color::blue:  name = "blue"; break;
        }
        return formatter<string_view>::format(name, ctx);
    }
};    
}
result = fmt::format("{}", check_color::blue);

This is probably a defect in msvc, but the change looks beneficial to me for msvc also in the non-modular case. I can see no reason, not to do it. There are no related warnings emitted if I crank the warning level up to maximum and/or enable DLL export. But I can't tell for sure in every usecase, therefor the WIP status.

Comment on lines 866 to 868
#if FMT_MSC_VER
= default
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest moving this into a separate macro FMT_MSC_DEFAULT that expands to = default if FMT_MSC_VER is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, just my thought. I wasn't sure if a single occurence warrants another macro. But now that it's blessed I will change it as suggested tomorrow.

@DanielaE DanielaE force-pushed the feature/inline-defaulted-destructors-for-msvc branch 2 times, most recently from 460716a to b24341c Compare April 28, 2021 05:36
applies to exception classes in case of msvc only
@DanielaE DanielaE force-pushed the feature/inline-defaulted-destructors-for-msvc branch from b24341c to 3afb5a8 Compare April 28, 2021 05:46
@DanielaE DanielaE marked this pull request as ready for review April 28, 2021 05:53
@vitaut vitaut merged commit d1a6e56 into fmtlib:master Apr 28, 2021
@DanielaE DanielaE deleted the feature/inline-defaulted-destructors-for-msvc branch April 28, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants