Tencent / rapidjson

A fast JSON parser/generator for C++ with both SAX/DOM style API
http://rapidjson.org/
Other
14.18k stars 3.53k forks source link

Add member does not use const string value in definition #1238

Open kikoalbiol opened 6 years ago

kikoalbiol commented 6 years ago

The suggested definition is:

GenericValue& AddMember(GenericValue& name, const std::basic_string& value, Allocator& allocator)

#if RAPIDJSON_HAS_STDSTRING
    //! Add a string object as member (name-value pair) to the object.
    /*! \param name A string value as name of member.
        \param value constant string reference as value of member.
        \param allocator    Allocator for reallocating memory. It must be the same one as used before. Commonly use GenericDocument::GetAllocator().
        \return The value itself for fluent API.
        \pre  IsObject()
        \note This overload is needed to avoid clashes with the generic primitive type AddMember(GenericValue&,T,Allocator&) overload below.
        \note Amortized Constant time complexity.
    */
    GenericValue& AddMember(GenericValue& name, std::basic_string& value, Allocator& allocator) {
        GenericValue v(value, allocator);
        return AddMember(name, v, allocator);
    }
#endif
kersson commented 4 years ago

Is there an explicit reason const wasn't used here? Without it, you can't use a const std::string for both the name and value. For example:

std::string str = "non-const string";
const std::string cStr = "const string";

Document d;
auto& alloc = d.GetAllocator();
d.AddMember(Value(cStr, alloc).Move(), str, alloc);  //< works
d.AddMember(Value(cStr, alloc).Move(), cStr, alloc); //< doesn't compile
pah commented 4 years ago

This looks like an oversight to me. Can you submit a pull-request?

// GenericObject:
GenericObject AddMember(ValueType& name, const std::basic_string<Ch>& value, AllocatorType& allocator) 
 { value_.AddMember(name, value, allocator); return *this; }

// GenericValue:
GenericValue& AddMember(GenericValue& name, const std::basic_string<Ch>& value, Allocator& allocator)
kersson commented 4 years ago

Looks like the reason it's this way is because if you add const std::basic_string& then you now can't resolve this case:

Document d;
auto& alloc = d.GetAllocator();

Value a("a");
Value b("b");

d.AddMember(a, "A", alloc);
d.AddMember(b, std::string("B"), alloc);

You get an error like this because "A" is convertible to StringRefType and const std::string&:

test.cpp: error: call to member function 'AddMember' is ambiguous
        d.AddMember(a, "A", alloc);
        ~~^~~~~~~~~
/Users/kersson/dev/rapidjson/include/rapidjson/document.h:1340:19: note: candidate function
    GenericValue& AddMember(GenericValue& name, StringRefType value, Allocator& allocator) {
                  ^
/Users/kersson/dev/rapidjson/include/rapidjson/document.h:1355:19: note: candidate function
    GenericValue& AddMember(GenericValue& name, const std::basic_string<Ch>& value, Allocator& allocator) {
                  ^

So I guess we're stuck with it? 😢