Analyzing some piece of code

Question

1.00/5 (1 vote)

See more:

Is this an OK way to do what it does?

C++

void CC::read(std::vector<SomeStructure> &info, Blob &data /*This is just some binary data imho*/)
{

 
   SomeStructure *ptr=(SomeStructure*) data.data();

 
    for(int i= 0;i<data.size()/sizeof(SomeStructure);i++)
    {
 
        info.push_back(*ptr);

 
        ptr++;
    }
}

Posted 3-Jul-13 4:02am

Dan page

Add a Solution

2 solutions

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

H.Brydon · Answer 1 · 2013-07-03T04:23:00

Solution 1

The answer to your question lies in the definition of the Blob class. The code looks suspect but nothing in what you have presented here looks completely wrong. To complete the question, the details of Blob need to be provided.

If Blob actually consists of a contiguous array of SomeStructure, you are going about the problem wrong. There will likely be some alignment details you are not considering. Consecutive elements in Blob will likely have some padding that you are not considering. This detail is determined by the compiler and can't be reliably predicted.

In any situation where you try to outsmart the compiler, you will usually lose.

Posted 3-Jul-13 4:23am

H.Brydon

Comments

Dan page 3-Jul-13 10:31am

Hi, the data inside the Blob class I think is stored as: void * data; (so it may indeed contain contigious array of SomeStructure objects).
This is not my code.
Sometimes they use this keyword in data structures: __packed (http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0491c/CJAFJHJD.html) - maybe that is how they defeat issues with the padding?
thanks.

H.Brydon 3-Jul-13 10:45am

Go for it. You might see differences between debug and release builds. Also different compiler versions.

Dan page 3-Jul-13 10:49am

hi H.Brydon, what do you mean with "Go for it"? as I mentioned this is not my code, I am analyzing reading someone else's code; I might need to modify it at times though.

H.Brydon 3-Jul-13 11:14am

Whoever wrote that should be shot. From a maintenance perspective, there are very minor changes that can be made to SomeStructure that will completely break this code. For example you can never add something to it that contains a pointer or method (or another class/struct containing a pointer or method). This code should not be used for persisting the struct/class. My guess is that it is for marshalling the data somewhere.

nv3 · Answer 2 · 2013-07-03T10:25:00

In my opinion this is an inefficient piece of code. As far as I understand it, it is supposed to do the following:

The Blob object contains binary data of known length which is a multiple of the length of SomeStucture. This entire chunk of data is copied into a vector with element type SomeStructure.

The code is inefficient for two reasons:

(a) The vector of SomeStructure is grown element by element. That requires multiple big copy operations that take place inside vector whenever the allocated space is exhausted.

(b) The operation push_back is executed with an element of SomeStructure as argument, which requires (depending on compiler optimization) multiple copy operations until the data arrives in the vector.

It would have been much more efficient to allocate the required size of the vector with a single operation and then to copy the entire blob with a single memcpy.

And all that under the assumption that the binary format of the Blob object is really compatible to an array of SomeStructure. If that code was in my project I would try to get rid of it.