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

Default parameter value used instead of the specified one #97

Closed
Lastique opened this issue Mar 3, 2020 · 11 comments · Fixed by #99
Closed

Default parameter value used instead of the specified one #97

Lastique opened this issue Mar 3, 2020 · 11 comments · Fixed by #99

Comments

@Lastique
Copy link
Member

Lastique commented Mar 3, 2020

Consider this test case:

#include <string>
#include <iostream>
#include <boost/parameter/keyword.hpp>

namespace keywords {

BOOST_PARAMETER_KEYWORD(tag, file_name)

}

void construct_impl(std::string const& str)
{
    std::cout << "str: " << str << std::endl;
}

template< typename Args >
void construct(Args const& args)
{
    construct_impl(args[keywords::file_name | std::string()]);
}

int main()
{
    construct((keywords::file_name = "sample.log"));
}

Currently, it prints "str: " instead of "str: sample.log". Boost.Parameter uses the default-constructed std::string value to pass to construct_impl instead of the one passed by user.

This causes regressions in Boost.Log (boostorg/log#104) and is reportedly started happening somewhere between Boost 1.69 and 1.71.

PS: Reproduced with gcc 9 and VS2015, but probably not only those compilers.

@urandon
Copy link

urandon commented Mar 3, 2020

@CromwellEnage is there any progress with issues #91 #95 #97? Should be release 1.71 and 1.72 be considered as totally broken and 1.70 is the only recent stable?

@Lastique
Copy link
Member Author

@CromwellEnage Please advise on the current plan to fix this issue. I don't want this kind of problem to be released in Boost.Log 1.73. If the problem cannot be fixed before 1.73, can we at least roll back Boost.Parameter to pre-1.71 state?

@glenfe
Copy link
Member

glenfe commented Mar 22, 2020

@Lastique It's clear Cromwell Enage isn't responding. Could you please prepare a PR to revert his changes back to pre-1.71 state? I'll take care of merging and shepherding them through the 1.73 release.

@Lastique
Copy link
Member Author

@glenfe So, will #99 be abandoned then? Should I close it?

@glenfe
Copy link
Member

glenfe commented Mar 22, 2020

@Lastique I think so. #99 fixes that one issue, while rolling back everything fixes more, right?

@Lastique
Copy link
Member Author

Yes. I was just thinking about merging it to develop first, then moving the result to a separate branch before reverting to 1.70.

@Lastique
Copy link
Member Author

@glenfe Created #100.

@sehe
Copy link

sehe commented Sep 3, 2020

I think this reintroduced #97, see #104? I'm lost in the web of reverts and coagulated merges. Please advise.

@Lastique
Copy link
Member Author

Lastique commented Sep 4, 2020

The revert to 1.70 state was eventually abandoned as it removed some functionality that was used in downstream libraries. The fix for this bug was merged to develop in #99. But apparently it was not merged to master so it was not released. @glenfe Could you merge to master, please?

@glenfe
Copy link
Member

glenfe commented Sep 5, 2020

@Lastique done.

@VladAdGad
Copy link

Still in 1.74 same issue

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 a pull request may close this issue.

5 participants