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

Add copy constructor for dynamic_format_arg_store #2432

Merged
merged 2 commits into from
Jul 23, 2021

Conversation

spyridon97
Copy link

@spyridon97 spyridon97 commented Jul 22, 2021

The goal of the PR is to develop a copy constructor (and default constructor) for the dynamic_format_arg_store.

The reason behind the need for a copy-constructor as stated in #2431 is that I would like to be able to instantiate a dynamic_format_arg_store object using another one to emulate a scope of arguments based on the class hierarchy of my code.

For the implementation of the copy-constructor, the members: data_ and named_info_ have been copied, while dynamic_args_ remained untouched because it also did not have a copy constructor or an iterator to implement a copy constructor.

Using the implementation included in this PR i was able to successfully do something like below:

        fmt::dynamic_format_arg_store<fmt::format_context> dArgs;

        std::vector<int> v1 = {5, 1235, 1235123};
        dArgs.push_back(fmt::arg("global_username", std::ref("utkarsh1")));
        dArgs.push_back(fmt::arg("global_hostname", std::ref("miron")));
        dArgs.push_back(fmt::arg("global_vector", v1));

        fmt::dynamic_format_arg_store<fmt::format_context> dArgs2(dArgs);
        dArgs2.push_back(fmt::arg("extra", std::ref("extra-value")));

Because of the new copy constructor, dArgs includes the first 3 variables, and dArgs2 includes the first 3 variables plus the 4th one.

Copy link
Contributor

@vitaut vitaut 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 the PR. A few small comments inline.

include/fmt/args.h Outdated Show resolved Hide resolved
include/fmt/args.h Outdated Show resolved Hide resolved
include/fmt/args.h Outdated Show resolved Hide resolved
include/fmt/args.h Outdated Show resolved Hide resolved
include/fmt/args.h Outdated Show resolved Hide resolved
@vitaut
Copy link
Contributor

vitaut commented Jul 22, 2021

Also please add a test case to https://github.com/fmtlib/fmt/blob/master/test/args-test.cc.

@spyridon97 spyridon97 force-pushed the dynamic-store-copy-constructor branch from afdc42c to 8fa46fe Compare July 22, 2021 14:58
include/fmt/args.h Outdated Show resolved Hide resolved
test/args-test.cc Outdated Show resolved Hide resolved
@spyridon97 spyridon97 force-pushed the dynamic-store-copy-constructor branch 2 times, most recently from 64d8f75 to 940496a Compare July 22, 2021 15:05
test/args-test.cc Outdated Show resolved Hide resolved
@spyridon97 spyridon97 force-pushed the dynamic-store-copy-constructor branch from 940496a to 55d3c48 Compare July 22, 2021 15:22
@vitaut
Copy link
Contributor

vitaut commented Jul 22, 2021

@DanielaE, do you know why msvc complains about test-module.ifc?

@DanielaE
Copy link
Contributor

The good news: Spiros' changes are fine, everything builds and all tests pass with msvc 16.10, 16.11-pre3 and 17.0-pre2. 🎉

The bad news: CI has a new windows-2019 image with CMake 3.21 instead of 3.20. The compiler is still the same (16.10.2)
The error message looks like the command line parameters for compiling fmt.cc as a module interface unit are missing with CMake 3.21. That's just a guess derived from the error messages without knowing the actual command line parameters in the compiler invocation! The builds on my local machine are still on CMake 3.20 (plus Ninja as supplied by msvc) where everything is fine with the default 26 parallel build threads in VSCode.

I will probably not find enough time today though to switch my local build environment to CMake 3.21 and look into differences in build.ninja generated by 3.20 and 3.21 🤔 Stay tuned!

@DanielaE
Copy link
Contributor

Everything builds locally with CMake 3.21! 😱
The compiler options are the same as before. But everything is switched from relative paths to absolute paths in build.ninja.
I have no clue how to proceed from here without seeing details and related files from the CI build ...

@spyridon97 spyridon97 closed this Jul 23, 2021
@spyridon97 spyridon97 reopened this Jul 23, 2021
@spyridon97
Copy link
Author

I accidentally closed this PR, so I reopened it.

Is there any progress as far as the Cmake issues? @DanielaE

@DanielaE
Copy link
Contributor

I just found out that the CMake generators "Visual Studio 16 2019" and "Visual Studio 17 2022" are 'improved' in CMake 3.21 in the sense that they are broken: they swallow the compiler option /interface in CMakeLists.txt line 31

  target_compile_options(${target}
      PRIVATE /interface /ifcOutput ${BMI}
      INTERFACE /reference fmt=${BMI})

Whatever I've tried to hide these compiler options from being processed by the generators fell flat. /interface is removed and /ifcOutput ... is converted into <ModuleOutputFile> ./fmt/build/test/test-module.ifc</ModuleOutputFile> in build/test/test-module.vcxproj. The latter is ok, but the former is 😱

@vitaut
Copy link
Contributor

vitaut commented Jul 23, 2021

In that case I suggest temporarily setting FMT_CAN_MODULE to OFF in

set(FMT_CAN_MODULE ON)
to unblock this PR.

@vitaut vitaut merged commit 3def950 into fmtlib:master Jul 23, 2021
@vitaut
Copy link
Contributor

vitaut commented Jul 23, 2021

Thank you!

@spyridon97 spyridon97 deleted the dynamic-store-copy-constructor branch July 24, 2021 08:53
@DanielaE
Copy link
Contributor

I just found out that the CMake generators "Visual Studio 16 2019" and "Visual Studio 17 2022" are 'improved' in CMake 3.21 in the sense that they are broken: they swallow the compiler option /interface in CMakeLists.txt line 31

A fix for that will hopefully land in CMake 3.21.1:

@vitaut
Copy link
Contributor

vitaut commented Jul 26, 2021

Thanks, @DanielaE, for looking into it!

@spyridon97
Copy link
Author

@vitaut I saw in the newest version of fmt this copy-constructor was deleted. Would it be possible to explain why?

@vitaut
Copy link
Contributor

vitaut commented Mar 22, 2022

IIRC it was broken, you can look up the commit history for more details.

@spyridon97
Copy link
Author

spyridon97 commented Mar 22, 2022

I looked it up, there was not a PR for the commit that did that (be51ee1) or further explanation apart from broken. I am asking because https://github.com/Kitware/ParaView relies on that.

Edit: I just saw #2653. so i am gonna follow up there,

@cdacamar
Copy link
Contributor

@spyridon97 do you know if the CMake issue was addressed which ran afoul with IFC generation?

@spyridon97
Copy link
Author

@spyridon97 do you know if the CMake issue was addressed which ran afoul with IFC generation?

Issue: https://gitlab.kitware.com/cmake/cmake/-/issues/22477 has been merged
Issue: https://gitlab.kitware.com/cmake/cmake/-/issues/22474 has a discussion which you can check

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.

4 participants