Author Topic: issue on NullLoader class  (Read 19251 times)

Offline blueshake

  • Regular
  • ***
  • Posts: 459
issue on NullLoader class
« on: December 01, 2009, 12:07:41 pm »
hello ,devs:
the  codes in Parser::Parse in parser file:
            if(!opts.loader) //this should always be true (memory will leak if a loader has already been initialized before this point)
                opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, true);


if I change the function Load() args from false(before) to true(the black one above). then the cc can not parse the codes correctly.

I think it is something relatived to these codes:
Code
NullLoader *nl = new NullLoader(file, (char*) s.c_str(), s.length() * sizeof(wxChar));
« Last Edit: December 01, 2009, 02:19:43 pm by blueshake »
Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?

Offline blueshake

  • Regular
  • ***
  • Posts: 459
Re: issue on NullLoader class
« Reply #1 on: December 02, 2009, 11:05:53 am »
Seem not cause any attention.

I have a question here.
here are the class LoaderBase's implementation.
Code
// ***** class: LoaderBase *****
class LoaderBase : public AbstractJob
{
    wxSemaphore sem;
    bool wait;

    void WaitReady()
    {
        if(wait)
        {
            wait = false;
            sem.Wait();
        }
    };

protected:
    wxString fileName;
    char *data;
    size_t len;

    void Ready()
    {
        sem.Post();
    };

public:
    LoaderBase() : wait(true), data(0), len(0) {};
    ~LoaderBase();
    void operator()() {};
    wxString FileName() const { return fileName; };

    bool Sync();
    char* GetData();
    size_t GetLength();
};

here the member variable data's type is char* ,why not wxChar*

when we do this
Code
NullLoader *nl = new NullLoader(file, (char*) s.c_str(), s.length() * sizeof(wxChar));
though the s.c_str() is convert to char*, it don't work here.

and in the body of bool Tokenizer::ReadFile() in tokenizer.cpp

these codes:
Code
if (m_pLoader)
    {
        fileName = m_pLoader->FileName();
        char* data  = m_pLoader->GetData();
        m_BufferLen = m_pLoader->GetLength();

        // the following code is faster than DetectEncodingAndConvert()
//        DetectEncodingAndConvert(data, m_Buffer);

        // same code as in cbC2U() but with the addition of the string length (3rd param in unicode version)
        // and the fallback encoding conversion
#if wxUSE_UNICODE
        m_Buffer = wxString(data, wxConvUTF8, m_BufferLen + 1); // + 1 => sentinel
        if (m_Buffer.Length() == 0)
        {
            // could not read as utf-8 encoding, try iso8859-1
            m_Buffer = wxString(data, wxConvISO8859_1, m_BufferLen + 1); // + 1 => sentinel
        }
#else
        m_Buffer = wxString(data, m_BufferLen + 1); // + 1 => sentinel
#endif

        success = (data != 0);
    }



 char* data  = m_pLoader->GetData(); some error happened and make the cc cannot parse the codes correctly.


Note:

if want to reproduce the issue,just change the false to true in the funciton  opts.loader=Manager::Get()->GetFileManager()->Load(bufferOrFilename, true);
Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?

Offline blueshake

  • Regular
  • ***
  • Posts: 459
Re: issue on NullLoader class
« Reply #2 on: December 02, 2009, 12:15:04 pm »
And I find that if the codes contain non-Englis letters, it will be not recognized by the parser.
Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: issue on NullLoader class
« Reply #3 on: December 02, 2009, 12:16:42 pm »
here the member variable data's type is char* ,why not wxChar*
Not sure why, but if wxChar works I see no reason not to use it. Did you try? It should run out-of-the-box.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline blueshake

  • Regular
  • ***
  • Posts: 459
Re: issue on NullLoader class
« Reply #4 on: December 03, 2009, 02:42:48 am »
and these codes may cause some problem.
the variable data point to the args buffer directly.but what will happened if the buffer is destroyed?
Code
class NullLoader : public LoaderBase
{
public:
    NullLoader(const wxString& name, char* buffer, size_t size) { fileName = name; data = buffer; len = size; Ready(); };
    void operator()(){};
};


and then check out these codes.
Code
    if(reuseEditors)
    {
        EditorManager* em = Manager::Get()->GetEditorManager();
        if(em)
        {
            wxFileName fileName(file);
            for(int i = 0; i < em->GetEditorsCount(); ++i)
            {
                cbEditor* ed = em->GetBuiltinEditor(em->GetEditor(i));
                if(ed && fileName == ed->GetFilename())
                {
                    wxString s(ed->GetControl()->GetText());
                    #if wxCHECK_VERSION(2, 9, 0)
                    NullLoader *nl = new NullLoader(file, (char*) s.wx_str(), s.length() * sizeof(wxChar));
                    #else
                    NullLoader *nl = new NullLoader(file, (char*) s.c_str(), s.length() * sizeof(wxChar));
                    #endif
                    return nl;
                }
            }
        }
    }

the wxString type varialbe s will be destroyed after s leave the if scope.and at this time,the data in class NullLoader will point to something which had been destroyed.
« Last Edit: December 03, 2009, 02:45:07 am by blueshake »
Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?

Offline blueshake

  • Regular
  • ***
  • Posts: 459
Re: issue on NullLoader class
« Reply #5 on: December 03, 2009, 07:02:11 am »
can somebody fixed this problem?

so I can upload my real-time parse patch.thanks.
Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: issue on NullLoader class
« Reply #6 on: December 03, 2009, 08:46:32 am »
can somebody fixed this problem?
How? As you see: It's nt that easy it seems. Probably Thomas (The creator of the NullLoader) can say something...?!
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: issue on NullLoader class
« Reply #7 on: December 03, 2009, 09:44:09 am »
I don't see what the issue is with NullLoader. NullLoader does what its name says, it loads nothing. It's a do-nothing dummy object with an empty operator() that can be inserted to FileManager's queue without making it crash. As a side effect, it also allows you to pass a file name and a byte buffer (pointer+size) to a callback function which is normally called after a file was loaded, so you don't have to implement the same function twice.

Yes, NullLoader's buffer member (inherited from LoaderBase) is a char*. That's what it should be. A loader does what it name says, it shuffles bytes from disk or some other medium (null, in this case) to some byte buffer. It does not know whether these bytes can be interpreted as unicode characters or not, and that's not its task either.
No, NullLoader (or LoaderBase for that matter) should not have a wxChar* buffer.

I haven't really looked in-depth what this is about, since it's code completion which I stay away from as far as I can, but here is an uneducated guess.
The first line in that snippet if(reuseEditors) looks like text that's in an editor is being sent to some other function (presumably one that parses the text). NullLoader doesn't care what the data is. The sending and the receiving end have to make sure that they talk about the same thing.
So, since you're saying that the data isn't read correctly, the probable reason is unicode text being copied from the editor and multibyte expected in the parser, or something similar.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline blueshake

  • Regular
  • ***
  • Posts: 459
Re: issue on NullLoader class
« Reply #8 on: December 03, 2009, 10:47:14 am »
Quote
So, since you're saying that the data isn't read correctly, the probable reason is unicode text being copied from the editor and multibyte expected in the parser, or something similar.

hello,Thomas
what is your way to solve this issue.

my friend VisualFC give me such a way to solve it.


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


VisualFC's  advice:
Code
    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();
    };

and change the associate calling.
Code
                if(ed && fileName == ed->GetFilename())
                {
                    //wxString s(ed->GetControl()->GetText());
                    //#if wxCHECK_VERSION(2, 9, 0)
                    //NullLoader *nl = new NullLoader(file, (char*) s.wx_str(), s.length() * sizeof(wxChar));
                    //#else
                    //NullLoader *nl = new NullLoader(file, (char*) s.c_str(), s.length() * sizeof(wxChar));
                    //#endif
                    NullLoader *nl = new NullLoader(file, ed->GetControl()->GetText());
                    return nl;
                }


yes , it work


so thomas,what is yuor opinion on these changes? thanks



Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: issue on NullLoader class
« Reply #9 on: December 03, 2009, 11:02:11 am »
Code
    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();
    };
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!
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9694
Re: issue on NullLoader class
« Reply #10 on: December 03, 2009, 11:04:31 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!
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)?
« Last Edit: December 03, 2009, 11:07:54 am by MortenMacFly »
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: https://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: https://wiki.codeblocks.org/index.php?title=FAQ

Offline blueshake

  • Regular
  • ***
  • Posts: 459
Re: issue on NullLoader class
« Reply #11 on: December 03, 2009, 12:32:13 pm »
Quote
Edit: Does strlen work here in case the buffer contains \00??? What about sizeof((const char*)str.mb_str()) (pseudo-code)?

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();
    };


what is your idea here?
Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: issue on NullLoader class
« Reply #12 on: December 03, 2009, 03:13:38 pm »
Quote
Does strlen work here in case the buffer contains \00??? What about sizeof((const char*)str.mb_str()) (pseudo-code)?
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.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline Jenna

  • Administrator
  • Lives here!
  • *****
  • Posts: 7255
Re: issue on NullLoader class
« Reply #13 on: December 03, 2009, 03:51:08 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.

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.

Offline blueshake

  • Regular
  • ***
  • Posts: 459
Re: issue on NullLoader class
« Reply #14 on: December 04, 2009, 06:00:57 am »
Quote
Nevertheless this code should be changed to something that works correctly (or removed), but I think it can make sense to have it.


Waiting for your good news.
Keep low and hear the sadness of little dog.
I fall in love with a girl,but I don't dare to tell her.What should I do?