Developer forums (C::B DEVELOPMENT STRICTLY!) > Development

issue on NullLoader class

<< < (3/5) > >>

MortenMacFly:

--- Quote from: MortenMacFly on December 03, 2009, 11:02:11 am ---This is no good, as e.g. the member variable len is no longer assigned., Thus NullLoader will be interface incompatible. Don't change the interface, or the usage of member variables from LoaderBase / AbstractJob. Their meaning and content must be consistent!

--- End quote ---
Ah wait - I saw your last line by now. Uh - multiple statements in one line... causes me a headache always... So it seems OK to me on second thoughts... ;-)

Edit: Does strlen work here in case the buffer contains \00??? What about sizeof((const char*)str.mb_str()) (pseudo-code)?

blueshake:

--- Quote ---Edit: Does strlen work here in case the buffer contains \00??? What about sizeof((const char*)str.mb_str()) (pseudo-code)?
--- End quote ---

hi,morten

no idea about this.can have a try.

Edit:
have a thought on "interface compatible".maybe we can overload the construct functioin NullLoader(const wxString& name, char* buffer, size_t size),I meant adding another function NullLoader(const wxString& name, const wxString& str)



--- Code: ---NullLoader(const wxString& name, char* buffer, size_t size)
         {
           fileName = name; data = buffer; len = size; Ready();
         };

    NullLoader(const wxString& name, const wxString& str)
    {
         //fileName = name; data = buffer; len = size; Ready();
        data = new char[str.Len()*2 + 1];
        strcpy(data, (const char*)str.mb_str());
        fileName = name; len = strlen(data);Ready();
    };
--- End code ---


what is your idea here?

thomas:

--- Quote ---Does strlen work here in case the buffer contains \00??? What about sizeof((const char*)str.mb_str()) (pseudo-code)?
--- End quote ---
No, of course it does not work. If anything, str.lenght()*sizeof(wxChar) is what you'd need.
However, this isn't the right approach, first you should not try to fix what isn't broken, and second, it would change the interface.

What's wrong is the way NullLoader is used, not the way it works. Actually having looked at it closer now, I'm surprised this code works at all, not so much because a pointer to a reference counted buffer that goes out of scope is returned, but because the aliased s.m_pchData is deleted twice.

The correct solution should look somewhat like allocating a char* buffer using operator new[] inside that function and the passing it to NullLoader.

Jenna:

--- Quote from: thomas on December 03, 2009, 03:13:38 pm ---What's wrong is the way NullLoader is used, not the way it works. Actually having looked at it closer now, I'm surprised this code works at all, not so much because a pointer to a reference counted buffer that goes out of scope is returned, but because the aliased s.m_pchData is deleted twice.

--- End quote ---

For me it crashes reliable when changing the mentioned parameter in Parser::Parse() to true.
and it crashes in LoaderBase's destructor, when data (as alias of s.m_pchData) should be deleted.

The Load(...)-function with reuseEditors == true seems to be used only by todolistview.cpp (if I see it correctly), but only if it parses a file that is not already opened in a builtin editor.

That means the NullLoader code in Filemanager::Load(...) will never be reached.
That's why we never had crashes because of it.

Nevertheless this code should be changed to something that works correctly (or removed), but I think it can make sense to have it.

blueshake:

--- Quote ---Nevertheless this code should be changed to something that works correctly (or removed), but I think it can make sense to have it.
--- End quote ---


Waiting for your good news.

Navigation

[0] Message Index

[#] Next page

[*] Previous page