Missing libemer::glow::GlowParameterBase::setValue(..) overload for const char*

[GoogleCodeExporter]

As the libember::glow::Value constructor is overloaded to take a const 
std::string& and a const char*, i would suggest to overload 
libemer::glow::GlowParameterBase::setValue(..) to take a const char* to avoid 
unexpected behaviour. See the attached patch with a proposal.

Original issue reported on code.google.com by nullable...@gmail.com on 7 May 2013 at 8:42

• Blocked on: Missing libemer::glow::GlowParameterBase::setValue(..) overload for libember::glow::Value #20

Attachments:

• Overload_setValue_const_char_pointer.patch

[GoogleCodeExporter]

In my opinion both overloads (constructor & setValue) are wrong, because:
1. char const* is implicitly convertible to std::string
2. There already is an overload for std::string rendering the char const* 
variants obsolete

What should be done is:
1. Remove the constructor overload for char const*
2. Change both std::string overloads to take their argument by value to enable 
copy elision.

Any opinions on this?

Original comment by Kimon.Ho...@lawo.com on 14 May 2013 at 11:21

[GoogleCodeExporter]

Is a fully matching signature not preferred over a signature where a conversion 
would be required to match?

I reported this because i came across the following code:

{
    libember::glow::GlowParameter p(0);
    p.setValue("HelloWorld");
    auto value = p.value();
    assert(value.type().value() == libember::glow::ParameterType::String);
}
{
    libember::glow::Value v("HelloWorld");
    assert(v.type().value() == libember::glow::ParameterType::String);
}

I would expect both assertions to hold, but both are failing, because the bool- 
overload was choosen by the compiler. This is really anoying, because it 
compiles and kind of works, but is not what i expect to happen.

I guess this would not change if passing the std::string argument by value - i 
think this would just be a possible optimization.

Original comment by nullable...@gmail.com on 14 May 2013 at 12:11

[GoogleCodeExporter]

> Is a fully matching signature not preferred over a signature where a 
conversion would be required to match?

Since the conversion is performed anyway it is cleaner (and, if passed by value 
also more efficient) to implicitly expose all conversions supported by the 
underlying type instead of explicitly replicating them in every scenario.

Of course this raises the second issue you mentioned about ordering which is 
governed by the coercion rules of the language.

I'll have a look into possible ways to solve this unexpected behavior.

Original comment by Kimon.Ho...@lawo.com on 14 May 2013 at 12:19

[GoogleCodeExporter]

I've looked at the code to refresh my memory and as it turns out that, except 
for GlowParameterBase::setValue(), all methods involved in the chain forwarding 
the set value to the underlying ber::Value instance are already templates.

Since ber::Value relies on the presence of a matching UniversalTagTraits<> 
specialization, my favored fix would boil down to:
1. Make GlowParameterBase::setValue() a template method, effectively delegating 
the conversion to ber::Value
2. Modify ber::Value::PayloadImpl<> to not literally use the passed type as the 
storage type, but instead delegate to choice of storage type to 
UniversalTagTraits<> by means of it's value_type typedef.
3. Add a "char const*" (and maybe also "char*", "char const[N]" and "char [N]") 
UniversalTagTraits specialization that maps to std::string as its value type.

Are there any objections to this solution? Especially about the fact that this 
solution makes GlowParameterBase::setValue() a template method?

Original comment by Kimon.Ho...@lawo.com on 16 May 2013 at 8:11

[GoogleCodeExporter]

I don't have any objections and like to see this solution.

Original comment by nullable...@gmail.com on 16 May 2013 at 12:03

[GoogleCodeExporter]

The template version would be ok for me, as long as the documentation for 
setValue(...) lists all valid argument types.

Original comment by p...@l-s-b.de on 17 May 2013 at 10:38

[GoogleCodeExporter]

The solution sounds good to me as well. But it would be nice when passing an 
unsupported type to the method would cause a compiler error.

Original comment by marius.k...@l-s-b.de on 17 May 2013 at 10:53

[GoogleCodeExporter]

I think an unsupported type will result in a compiler error with the proposed 
solution, because the corresponding trait specialization is missing.

Original comment by nullable...@gmail.com on 17 May 2013 at 2:06

[GoogleCodeExporter]

Original comment by Kimon.Ho...@lawo.com on 26 May 2013 at 2:10

• Added labels: Milestone-Release1.4

[GoogleCodeExporter]

The only type no supported by the current implementation that would be accepted 
by the proposed generic solution is ObjectIdentifier.
According to the documentation a relative object identifier is not a valid 
parameter value, so this is a problem.

One solution I'd see is creating an unary metafunction predicate that states 
whether a supplied universal tag is acceptable. The universal tag is in turn 
obtained from the supplied C++ type by means of the UniversalTagTraits template.

To be slightly nit-picky about this topic: The current implementation also 
accepts boolean values, although the boolean type is not listed as a valid 
parameter type in the documentation.

Original comment by Kimon.Ho...@lawo.com on 27 May 2013 at 11:20

[GoogleCodeExporter]

Original comment by Kimon.Ho...@lawo.com on 15 Jul 2013 at 3:07

• Added labels: Milestone-Release1.6
• Removed labels: Milestone-Release1.4

[GoogleCodeExporter]

Original comment by Kimon.Ho...@lawo.com on 26 Feb 2014 at 4:37

• Added labels: Milestone-Release1.8
• Removed labels: Milestone-Release1.6

[GoogleCodeExporter]

Original comment by Kimon.Ho...@lawo.com on 26 Feb 2014 at 4:44

• Now blocked on: Missing libemer::glow::GlowParameterBase::setValue(..) overload for libember::glow::Value #20

[GoogleCodeExporter]

Original comment by Kimon.Ho...@lawo.com on 25 Jun 2014 at 10:46

• Added labels: Component-LibEmber

[mkeuck]

Since this issue will (probably) be resolved with the upcoming C++ 17 version I would like to close this entry. If this is a requirement for the 1.8 release, please reopen this issue.