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:
NullLoader *nl = new NullLoader(file, (char*) s.c_str(), s.length() * sizeof(wxChar));
Seem not cause any attention.
I have a question here.
here are the class LoaderBase's implementation.
// ***** 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
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:
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);
And I find that if the codes contain non-Englis letters, it will be not recognized by the parser.
(http://www.ikaca.sh.cn/attachment/200912/2/1745_1259751319Hllc.jpg)
Quote from: blueshake on December 02, 2009, 11:05:53 AM
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.
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?
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.
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.
can somebody fixed this problem?
so I can upload my real-time parse patch.thanks.
Quote from: blueshake on December 03, 2009, 07:02:11 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...?!
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.
QuoteSo, 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 : NullLoader(const wxString& name, char* buffer, size_t size) { fileName = name; data = buffer; len = size; Ready(); };VisualFC's advice: 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.
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
Quote from: blueshake on December 03, 2009, 10:47:14 AM
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!
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!
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)?
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)
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?
QuoteDoes 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.
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.
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.
QuoteNevertheless 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.
Quote from: blueshake on December 04, 2009, 06:00:57 AM
QuoteNevertheless 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.
To make it clear: the code to change is not the NullLoader code (in my opinion), but the code that uses it.
But I don't think I find the time to dig into it soon.
it is Ok if NullLoader class can load the text from the Editor when I set the args to true.
Anyway thanks your effort.
This snippet (FileManager::Load) copies the bytes, m_pchData points to, into NullLoader's data, so no crash anymore (I'm not sure if everything is correct, as written before I have no time, to work on it seriously).
if(ed && fileName == ed->GetFilename())
{
wxString s(ed->GetControl()->GetText());
int len = s.length() + 1;
NullLoader *nl = new NullLoader(file, (char*)wxStrcpy(new wxChar[len], s.c_str()), len * sizeof(wxChar));
return nl;
}
But you get a problem here:
If I see it correctly, FileLoader loads the content of the files into data (byte by byte from disk), but the editors hold a widechar-string (at least in unicode-builds), so loading a file from disk or via fileloader does not lead to the same content automatically and parsing might fail (happens here for example).
hi,jens:
just as what visualFc do ,what about using this way?
NullLoader *nl = new NullLoader(file, strcpy(new char[len* sizeof(wxChar)], (const char*)s.mb_str()), len * sizeof(wxChar));
Quote from: blueshake on December 04, 2009, 11:18:35 AM
it is Ok if NullLoader class can load the text from the Editor when I set the args to true.
No, because then it is not a NullLoader any more. However, if you insist of doing a fix on loader level, you can write a class like this:
class EditorReuser : public LoaderBase
{
EditorReuser(cbEditor const& e)
{
wxString s(e.GetControl()->GetText());
fileName = e.GetFilename();
data = new char[s.length() * sizeof(wxChar)];
len = s.length() * sizeof(wxChar);
Ready();
}
void operator()(){};
};Then use a
EditorReuser instead of a
NullLoader.
Note: not checked for typos or correctness.
Edit: also note that you may have to convert the editor's text, since that will be Unicode characters, not UTF-8 or some Windows codepage (which the parser might expect). Or, you will have to tell the parser that the encoding is Unicode.
Thanks. I will have a try when I get my time.
hi,devs:
I think we can learn something from file.h of wxWidget.
code snippet:
bool Write(const wxString& s, const wxMBConv& conv = wxConvUTF8)
{
const wxWX2MBbuf buf = s.mb_str(conv);
if (!buf)
return false;
size_t size = strlen(buf);
return Write((const char *) buf, size) == size;
}