0

I have code like this:

ByteArray ret;
ret.resize( MAX( body_left, tmp_read.size() ) );
while ( body_left > 0 ) {
    ByteArray::Write r = tmp_read.write();
    int rec = 0;
    err = connection->get_partial_data( r.ptr(), MIN( body_left, tmp_read.size() ), rec );
    if ( rec > 0 ) {
        ByteArray::Write w = ret.write();
        copymem( w.ptr(), r.ptr(), rec );
        body_left -= rec;
    }
}

I find it challenging to understand this code. A few questions:

Is ret.resize(MAX(body_left,tmp_read.size())); allocating the ByteArray of highest body_left or tmp_read.size()?

In ByteArray::Write r = tmp_read.write(); does r become a pointer to location in space that is going to be used to write data?

In ByteArray::Write w = ret.write();, does w become a pointer like r in the previous question?

Also, in this line:

copymem(w.ptr(),r.ptr(),rec);

As I understand this line, all of the data that is gathered under pointer r is copied to location under the pointer w. The problem is that they are different size, how to move pointer w.ptr() to keep data intact and in correct order? Or is w.ptr() is a pointer to function and this should not be a problem.


Extra context:

Method get_partial_data returns chunks of data - lets say 20, 20, and 10 bytes each. Variable ret is supposed to be 50 bytes long and have those chunks merged into one ByteArray.

Unfortunately I cannot find the definition of ByteArray in this project, so I guess it's part of another library (libGL maybe?).

I know this question is not very precise and I am making a leap of faith, but if anyone can assist me I would be grateful.

Original class and project this code was taken from:

https://github.com/okamstudio/godot/blob/master/core/io/http_client.cpp

Lines 503-516.

It's in different shape, as I already have applied dirty hack (that does not work to well).

Drachenfels
  • 3,037
  • 2
  • 32
  • 47
  • If it makes you feel better, I'm a reasonably proficient C++ developer (even if I say so myself), but this code doesn't make much sense to me, either. It uses some kind of a class library I've never seen before. – Igor Tandetnik Aug 21 '14 at 01:35
  • Where did you find the code? –  Aug 21 '14 at 01:41
  • Hah! :) Thanks guys, I am updating question with link to project and some extra data. – Drachenfels Aug 21 '14 at 01:41
  • [ByteArray is just a typedef.](https://github.com/okamstudio/godot/search?l=cpp&q=bytearray&utf8=%E2%9C%93) And [write()](https://github.com/okamstudio/godot/blob/9ff6d55822647c87eef392147ea15641d0922d47/core/dvector.h) doesn't return a pointer, so they're not pointers I think. –  Aug 21 '14 at 01:46
  • As for copymem, it's a [macro](https://github.com/okamstudio/godot/blob/9ff6d55822647c87eef392147ea15641d0922d47/core/os/copymem.h). I'm not that well versed in C++ so somebody else can answer your question. –  Aug 21 '14 at 01:52
  • Thank you very much so far, at least I have some basics explained. :) – Drachenfels Aug 21 '14 at 01:57

2 Answers2

1

Is ret.resize(MAX(body_left,tmp_read.size())); allocating the ByteArray of highest body_left or tmp_read.size()?

MAX most likely is a macro that returns the greater of the two arguments. The line ret.resize(MAX(body_left,tmp_read.size())); ensures that ret is large enough for whatever writing operations that may occur.

In ByteArray::Write r = tmp_read.write(); does r become a pointer to location in space that is going to be used to write data?

In ByteArray::Write w = ret.write();, does w become a pointer like r in the previous question?

Write is a class defined on line 187. write() is a function defined on line 209 that returns a Write object, not a pointer. Therefore r and w are never pointers.

class Write {
  // ...
};

Write write() {
  Write w;
  // ...
  return w; 
}

Also, in this line:

copymem(w.ptr(),r.ptr(),rec);

As I understand this line, all of the data that is gathered under pointer r is copied to location under the pointer w. The problem is that they are different size, how to move pointer w.ptr() to keep data intact and in correct order? Or is w.ptr() is a pointer to function and this should not be a problem.

copymem is a macro defined on line 36.

#define copymem(m_to,m_from,m_count) \
do { \
  unsigned char * _from=(unsigned char*)m_from; \
  unsigned char * _to=(unsigned char*)m_to; \
  int _count=m_count; \
  for (int _i=0;_i<_count;_i++) \
    _to[_i]=_from[_i]; \
} while (0);

All this code appears to do is copy the contents of m_from to m_to. get_partial_data feeds the amount to read into rec, which is passed to copymem as m_count.

  • There is still one thing that is unclear for me, line: copymem(w.ptr(),r.ptr(),rec), assuming that w.ptr() is a pointer to location in memory that consists our data. Would be that sufficient to do set of operations like that: copymem(w.ptr(),r.ptr(),rec), copymem(w.ptr() + 20,r.ptr(),rec), copymem(w.ptr() + 40,r.ptr(),rec) (assuming we received 20, 20 and 10 bytes like in original question I am asking) – Drachenfels Aug 21 '14 at 10:57
  • Well maybe I will add helper question. Line like that: unsigned char * _from=(unsigned char*)m_from, is it creating new variable that will be a pointer to unsigned char and pointer value (location) is calculated as a cast to pointer unsigned char from anything that m_from can be? – Drachenfels Aug 21 '14 at 11:30
  • you are my life saviour. Thank to yours explanations I figured out everything. Maybe my solution is not the best but works. I am only concerned with memory leaks right now. Not sure how to detect those. I will post my code, if anyone could take a look and confirm that my code is all right in terms you have seen much worse and it does the job without violating every possible good practice rule in C++ world out there. – Drachenfels Aug 21 '14 at 14:51
0

To wrap everything I (we) have learnt so far:

Original code was as follows:

    ByteArray::Write r = tmp_read.write();
    int rec=0;
    err = connection->get_partial_data(r.ptr(),MIN(body_left,tmp_read.size()),rec);
    if (rec>0) {
        ByteArray ret;
        ret.resize(rec);
        ByteArray::Write w = ret.write();
        copymem(w.ptr(),r.ptr(),rec);
        body_left-=rec;
        if (body_left==0) {
            status=STATUS_CONNECTED;
        }
        return ret;
    }

Where copymem is a macro that looks as follows:

    #define copymem(m_to,m_from,m_count) \
    do { \
      unsigned char * _from=(unsigned char*)m_from; \
      unsigned char * _to=(unsigned char*)m_to; \
      int _count=m_count; \
      for (int _i=0;_i<_count;_i++) \
        _to[_i]=_from[_i]; \
    } while (0);

Using everything that remyable explained to me I dropped macro usage, because as I understand w.ptr() returns section of the memory and macro always starts coping from the first byte of that section. Macro do not support offset-ing nor passing pointers.

End result looks as follows:

    ByteArray ret;
    ret.resize(MAX(body_left,tmp_read.size()));
    ByteArray::Write w = ret.write();
    unsigned char * _to = (unsigned char*) w.ptr();
    int _offset = 0;
    while (body_left > 0) {
        ByteArray::Write r = tmp_read.write();
        int rec=0;
        err = connection->get_partial_data(r.ptr(),MIN(body_left,tmp_read.size()),rec);
        if (rec>0) {
            unsigned char * _from=(unsigned char*)r.ptr();
            for (int _i=0;_i<rec;_i++)
                _to[_offset+_i]=_from[_i];
            _offset += rec;
            body_left-=rec;
        }
    }
    if (body_left==0) {
        status=STATUS_CONNECTED;
    }
    return ret;

Anyone can confirm this is viable solution or even suggest improvement?

Update. I found that I can actually move w.ptr() by offset for a macro, alternative code looks as follow:

    ByteArray ret;
    ret.resize(MAX(body_left,tmp_read.size()));
    ByteArray::Write w = ret.write();
    int _offset = 0;
    while (body_left > 0) {
        ByteArray::Write r = tmp_read.write();
        int rec=0;
        err = connection->get_partial_data(r.ptr(),MIN(body_left,tmp_read.size()),rec);
        if (rec>0) {
            copymem(w.ptr()+_offset,r.ptr(),rec);
            body_left-=rec;
            _offset += rec;
        }
    }
    if (body_left==0) {
        status=STATUS_CONNECTED;
    }
    return ret;

Comments/opinions?

Drachenfels
  • 3,037
  • 2
  • 32
  • 47