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

DeepCopy? CMake? #1

Closed
ArtemGr opened this issue Mar 29, 2014 · 27 comments
Closed

DeepCopy? CMake? #1

ArtemGr opened this issue Mar 29, 2014 · 27 comments
Labels

Comments

@ArtemGr
Copy link

ArtemGr commented Mar 29, 2014

Hi, Philipp!

Would you mind accepting a pull request of this patch
https://github.com/scanlime/rapidjson/commit/0c69df5ac098640018d9232ae71ed1036c692187 ?

Also, what do you think of Git and CMake patches at https://github.com/rjeczalik/rapidjson ? Would it be feasible to integrate these in order to have both your fixes and Git submodules and CMake?

@pah
Copy link
Owner

pah commented Mar 31, 2014

Thanks for your suggestions (and for mentioning this fork in the upstream issue tracker).
In general, I'm not happy with the current proliferation of source repositories, although I didn't help by forking myself. ;-)

Would you mind accepting a pull request of this patch scanlime/rapidjson@0c69df5 ?

I'm not sure. In general, deep-copying is a valuable addition, but I don't like the API of this proposal. A couple of weeks ago, I have built a similar solution which I didn't publish on GitHub so far.

I have now pushed it and opened the pull-request #2 for it. What do you think? Adding a DeepCopy function to GenericDocument to follow the patch in scanlime/rapidjson@0c69df5 would be possible.

@scanlime, what do you think? Is there an upstream issue reported for this?

Also, what do you think of Git and CMake patches at https://github.com/rjeczalik/rapidjson ?
Would it be feasible to integrate these in order to have both your fixes and Git submodules and CMake?

I would certainly accept and merge the Cmake integration. Although I don't use the Rapidjson build infrastructure at all (instead maintain the Rapidjson sources directly within our projects), this may be useful for some people.

I can see the value of the Git submodules approach, but I want to follow upstream on this (see svn/trunk branch). I would accept patches to update the thirdparty sources, though.

@pah pah added the question label Mar 31, 2014
@ArtemGr
Copy link
Author

ArtemGr commented Mar 31, 2014

Your version of DeepCopy looks good!
Seen your other Tencent/rapidjson#1. Much appreciated. Wasn't aware that Milo had a repository here.

CMake integration isn't as important as I've initially thought, because it's mostly used to build the unit tests. With your Tencent/rapidjson#1 I'd rather wait for Milo reaction.

@ArtemGr ArtemGr closed this as completed Mar 31, 2014
@michcleary
Copy link

Is this DeepCopy in rapidjson version 1.1.0? Is there a good example of it? Otherwise I might just use http://stackoverflow.com/questions/22707814/perform-a-copy-of-document-object-of-rapidjson the copyDocument comment/answer.

@pah
Copy link
Owner

pah commented Feb 15, 2017

Yes, all of the changes in this repository are upstreamed by now. DeepCopy is called CopyFrom in the upstream API:

You can find the CopyFrom documentation here and an example in the tutorial.

@michcleary
Copy link

michcleary commented Feb 15, 2017 via email

@pah
Copy link
Owner

pah commented Feb 15, 2017

CopyFrom already does the traversal, as does the explicit copy constructor (which requires an allocator for the allocation of the string and value copies).

Just use something like

Document src;
src.Parse(...);

Document dst;
dst.CopyFrom(src, dst.GetAllocator());
// dst holds the same values (recursively) as src

Submitted upstream by myself as Tencent/rapidjson#20.

@michcleary
Copy link

michcleary commented Feb 16, 2017 via email

@michcleary
Copy link

michcleary commented Feb 16, 2017 via email

@pah
Copy link
Owner

pah commented Feb 16, 2017

Hi Michele,

I would appreciate if you could try adding some formatting to your answer. It's very hard to parse it here.

Starting from your original JSON:

{ 
  "type": "SlxROI",
  "m_position": { "m_x": 16, "m_y": 32, "m_z": 0 },
  "m_size": { "m_width ":64," m_height ":128," m_depth ":3}
}

you can access the m_position member (assuming you know it's there), via

   using namespace rapidjson;
   Document origDocument; 
    // ...
   assert( origDocument.IsObject() && origDocument.HasMember("m_position");
   Value& m_pos = origDocument["m_position"];
   assert( m_pos.IsObject() );
   for (Value::ConstMemberIterator itr = m_pos.MemberBegin(); itr != m_pos.MemberEnd(); ++itr)
   {
        printf("Value of 'm_position' member '%s' is %u\n",
            itr->name.GetString(), itr->value.GetUint());
   }

Dereferencing the ConstMemberIterator returns a Member object, which essentially contains a name and a value element.

Hope that helps,
Philipp

@michcleary
Copy link

michcleary commented Feb 17, 2017 via email

@michcleary
Copy link

michcleary commented Feb 27, 2017 via email

@michcleary
Copy link

michcleary commented Feb 27, 2017 via email

@pah
Copy link
Owner

pah commented Apr 5, 2017

By the way, I want to copy the Value without making a Document. We are trying to keep the DOM concept out of our design for now (except for error checking) and just create Nodes/Values. That would be great if you could let me know a good way to do this. I'm sure you would have a copy concept for this like you do for the Document.

You can't have a Value without an Allocator. With an Allocator, you can use the same concept even without a Document. A Document is more or less just a combination of an allocator and a (root) Value.

@michcleary
Copy link

michcleary commented Apr 6, 2017 via email

@pah
Copy link
Owner

pah commented Apr 11, 2017

How would I add a name value pair without a document object that I'm keeping?

As I said: You need to supply an allocator.

The simplest solution is to use the CrtAllocator instead of the MemoryPoolAllocator through a simple global instance:

typedef rapidjson::GenericValue<rapidjson::UTF8<>, rapidjson::CrtAllocator> JsonValue;

rapidjson::CrtAllocator& jsonAllocator()  {
  static rapidjson::CrtAllocator instance;
  return instance;
}

 // use jsonAlloctor() wherever you need one
JsonValue val = JsonValue( n, v, jsonAllocator() );

@michcleary
Copy link

michcleary commented Apr 11, 2017 via email

@pah
Copy link
Owner

pah commented Apr 12, 2017

Do you mean GenericValue instead of rapidjson::Value

Yes, obviously. I already replaced Allocator by CrtAllocator in the sources above.

@michcleary
Copy link

michcleary commented Apr 16, 2017

If I want to Deep Copy a JsonValue, how would that be?

I'm trying
static void copyVal(JsonValue & newNode, JsonValue & originalNode)
{
newNode.CopyFrom(originalNode, newNode.GetAllocator()); //can't use GetAllocator with JsonValue
}

I tried this too:
newNode.CopyFrom(originalNode, jsonAllocator()); //non-static member reference must be specific to a specific object

Any ideas?

@pah
Copy link
Owner

pah commented Apr 16, 2017

It looks like you have defined jsonAllocator() inside a class? Can you try defining it as a free function instead?

@michcleary
Copy link

Cool! It seems to compile with
newNode.CopyFrom(originalNode, jsonAllocator());

once I made copyValue method not be static anymore. It could use jsonAllocator(). Thanks!

@michcleary
Copy link

michcleary commented Apr 16, 2017

I'm trying to take an existing object, and get the name/value pair out of it to use in a new object.

The object was created elsewhere:

JsonValue name = JsonValue("name", jsonAllocator());
JsonValue value = JsonValue(true, jsonAllocator()); //could be any type
m_jsonVal = JsonValue(kObjectType);
m_jsonVal = AddMember(name, value, jsonAllocator());

Then elsewhere I need to get value out to re-add it with addMember. Is there a good way to do this?
I have m_jsonVal, but don't have the individual JsonValue's that were the name/value in it.

Like, I have m_node, which is a kObjectType, and want to add the name/value pair in m_jsonVal to it.
I can't do this: m_jsonVal.AddMember(m_jsonVal, jsonAllocator()); //no constructor exists

//So I'm trying
JsonValue copyParam; //object is name/value in it
copyParam.CopyFrom(param, jsonAllocator());
JsonValue::MemberIterator iter = copyParam.MemberBegin();
JsonValue key = JsonValue(iter->name.GetString(), jsonAllocator());
//but I'm having trouble getting the value, since it could be any type.

//is there an easy way to do that? It would be great to do this, but this isn't doing the trick either:
JsonValue temp = JsonValue(copyParam.Get(), jsonAllocator()); //can't deduce template arg for T

@pah
Copy link
Owner

pah commented Apr 16, 2017

I think, these type of questions are better suited for Stackoverflow, as I can't give hands-on support here.

m_jsonVal.AddMember(m_jsonVal, jsonAllocator()); //no constructor exists

AddMember always takes a key, a value, and an allocator. And beware that when passing the parameters as JsonValue, the source objects are emptied during this operation, see move semantics in the documentation.

Why don't you simply do (assuming you don't need copyParam later):

m_jsonVal.AddMember( iter->name, iter->value, jsonAllocator() );

@pah
Copy link
Owner

pah commented Apr 17, 2017

It looks like when I try to set up the iterator, it's crashing. Rapidjson thinks it's not an object.

I'm pretty sure, you messed up the value earlier due to the move semantics. You should read up on this.

@ArtemGr
Copy link
Author

ArtemGr commented Apr 17, 2017

Nobody answers on Stackoverflow. I'm going to lose my job if I don't get this done by Thursday.

There's working hard and there's working efficiently.
Take a deep breath, step back and see if you can solve the problem differently.
Maybe inform the "work" about the problem and the alternatives.

If you're set on making the rapidjson work in this particular way, instead of adjusting the algorithm, then follow the best practices by making a test case (aka http://stackoverflow.com/help/mcve) and using a debugger. Right now you remind me of someone trying to find a black cat in a dark room and asking others to help him do that. Turn on the light! Isolate the problem with MCVE, isolate it further by getting a debugging backtrace from that MCVE, then let the others help you.

If you can't do those simple things, then maybe you ought to loose your job and do something else for a living, if you don't mind me saying.

@michcleary
Copy link

michcleary commented Apr 17, 2017

I'm trying to make my code more like the uses I'm seeing online, like not using a class JsonValue pointer. I'm trying to use a class document, and add name/value pairs to that.

When I was reading about move semantics, the examples they give similar to my way say "without sufficient lifetime". I see this example , which is similar to what I need to do and would be ok to adopt in my algorithm, but what he said to do Document tempDoc(*m_document.GetAllocator()), is not compiling. Illegal indirection. VS says no operator * matches these operands.

@ArtemGr
Copy link
Author

ArtemGr commented Apr 18, 2017

Could you take a look at please? #8

I don't know about Philipp, but for me a bunch of code that wasn't organized into MCVE is a non-starter.
Thanks for taking your question out of this issue, though.

@ArtemGr
Copy link
Author

ArtemGr commented Apr 21, 2017

Nobody answers on Stackoverflow.

Free advice.

  1. Make an MCVE (if you can't figure it out, MCVE is when your piece of code is minimal yet can be compiled and run "as is", without spending the time to add the headers, int main, figuring the unknowns, et cetera).
  2. Give the URL of the library you're using (e.g. https://github.com/miloyip/rapidjson), so that people would know how to run your code.
  3. Use a language tag in order to attract more eyes (put "c++" and "rapidjson" tags on the question).
  4. Put a bounty on your question.

Together this should greatly increase the chance of getting an answer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants