Proposal: add GetLastError() to wxXmlDocument (Issue #24167)
Randalphwa
Description
When wxXmlDocument::Load() fails, it generates a wxLogError() message with the description of the error and the line number in the file it was trying to load when the error occurred. That's quite useful if you are running a Debug build of your application where you can access logging. A Release build may not have logging enabled, but it can still be useful to get that error message. The information is, in fact, available to the Load() function in Release builds, it's just not available to the caller.
A simple way to save the information for the caller to retrieve would be to add to the header file:
public: const wxString& GetLastError() const { return m_err_msg; } // Returns error message from last Load() call. private: wxString m_err_msg;
In xml.cpp, if there is a failure, you could call:
m_err_msg.Printf(_("XML parsing error: '%s' at line %d"), error.c_str(), (int)XML_GetCurrentLineNumber(parser));
I've actually done this in a private build, and verified that the the error string does get correctly created with description and line number in a release build.
So my question is, would this be worth submitting a PR for?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I agree that having access to the error would be useful, but I dislike having errno-like API. What about adding
struct wxXmlParseError { wxString message; int line; };
and adding wxXmlParseError* error = nullptr parameter to Load()? If it is non-null, the error would be returned in it, otherwise we'd use wxLogError() as we currently do.
Another alternative would be to extract error reporting in a new virtual DoReportError(const wxString&, int) which would call wxLogError() by default but could be overridden, but it seems a bit weird to derive from wxXmlDocument -- even if it does have virtual functions, for whatever reason.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Currently the line number gets printed into the log message. I'm not sure having the line available as an actual variable would be useful, on the assumption that the XML file cannot be fixed programmatically. If we keep the number printed in the error message, then we could simplify this by simply adding a wxString* parameter:
bool wxXmlDocument::Load(wxInputStream& stream, const wxString& encoding, int flags, wxString* err_msg = nullptr) { ... if (!XML_Parse(parser, buf, len, done)) { wxString error(XML_ErrorString(XML_GetErrorCode(parser)), *wxConvCurrent); if (err_msg) err_msg->Printf(_("XML parsing error: '%s' at line %d"), error.c_str(), XML_GetCurrentLineNumber(parser));
I'd be inclined to leave the wxLogError() in place whether or not the err_msg pointer is supplied. The caller may have a different use for the error message such as presenting that information to the application's user who might not be presented with the log message.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Currently the line number gets printed into the log message. I'm not sure having the line available as an actual variable would be useful,
In theory, I can easily see how it would be useful: you could e.g. open the XML file in an editor and position the cursor at this line.
I'd be inclined to leave the wxLogError() in place whether or not the err_msg pointer is supplied.
The problem is that this behaviour can't be easily overridden, i.e. if you don't want the error message to be logged you'd have to resort to global hacks with wxLogNull. While if the message is not given but you would like to output it, it's pretty trivial to do it from the application code.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
In the current code base, you cannot prevent wxLogError() from being called:
if (!XML_Parse(parser, buf, len, done)) { wxString error(XML_ErrorString(XML_GetErrorCode
(parser)),
*wxConvCurrent);
wxLogError(_("XML parsing error: '%s' at line %d"),
error.c_str(),
(int)XML_GetCurrentLineNumber(parser));
ok = false;
break;
}So if I'm understanding correctly, if a a 4th parameter is added (ptr to something) then wxLogError should only called if that ptr is null as in:
if (!XML_Parse(parser, buf, len, done)) { if (new_ptr == nullptr) { wxString error(XML_ErrorString(XML_GetErrorCode(parser)), *wxConvCurrent); wxLogError(_("XML parsing error: '%s' at line %d"), error.c_str(), (int)XML_GetCurrentLineNumber(parser)); } else { // do something with new_ptr } ok = false; break; }
Good point on using the line number to position the cursor -- one could parse it out of the error string, but it would certainly be easier via a structure.
As an aside, XML_GetCurrentLineNumber() returns an unsigned value -- 0 is used to indicate an error. It's either going to be unsigned long long or just unsigned long depending on if XML_LARGE_SIZE is defined. Casting it to (int) might be fine for wxLogError() which is formatting it into a string, but I personally think it would be a mistake to use int in the structure for a value that is unsigned. I'd be inclined to go with unsigned long using a static_cast to avoid any potential warning about truncation. At least I think it's reasonable to assume that parsing will fail for memory issues long before it reads 4,294,967,295 lines of XML in a 32-bit app...
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I used to be a big fan of unsigned (and it unfortunately shows in wx API design) because I thought that it was always better to use it for values that are always positive, but I changed my mind quite a few years ago already and now prefer to use int for everything except very special cases.
Rationale can be found in several C++ Core Guidelines items, e.g. ES.106, and, more generally, the use of unsigned size_t in STL is recognized as a big mistake by all C++ experts.
TL;DR: let's use int for the line number.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
I've noticed something else odd about the Load function. It passes in the encoding to use, but then ignores it:
bool wxXmlDocument::Load(wxInputStream& stream, const wxString& encoding, ...) { (void)encoding; ... ctx.encoding = wxS("UTF-8"); // default in absence of encoding=""
The documentation for wxXmlDocument::Load does not mention that the encoding parameter is ignored. It also doesn't mention that it would be ignored anyway if the XML file contains an encoding declaration. An XML that does not have an explicit encoding normally defaults to UTF-8 anyway, which is probably why I'm not seeing any issues raised over this.
At the very least, I'm inclined to update the documentation to mention that the encoding parameter is ignored while I'm adding the documentation for the additional parameter. The other option would be to create an additional method for 3.3 only that doesn't use that parameter (actually two methods, since there is one for a stream and one for a filename).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
It looks like "encoding" here was only ever meant to be used as the encoding of the string read from the file, not the encoding of the file itself, see the code such as it was before 4519d8e (Remove wxUSE_UNICODE checks as they're always true now, 2022-10-27).
This commit was incomplete, BTW, as it should have also removed wxXmlParsingContext::conv. I've done this now in #24179 and applied a few more cleanups, including deprecating the functions taking encoding as it clearly was confusing.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Since you are doing a bunch of work on wxXmlDocument in your xml-conv-simplify branch anyway, feel free to add in the changes discussed in this issue, since they fit in with what you are doing anyway. Otherwise, I'll wait until that branch is merged and then add these changes on top of it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
I really didn't plan to work on it, I just went to the code to answer your question and realized that encoding parameter was very confusing, so I wanted to remove it, but to do it easily I had to simplify other things before, so this is what I did.
So I'll merge that PR soon and let you work on the rest of it. TIA!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Closed #24167 as completed.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Randalphwa
Closing this issue since both PRs have been merged, and I have verified that the new wxXmlParseError structure is providing the needed error information.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.