search

lestrrat-go / libxml2

Interface to libxml2, with DOM interface
MIT License
230 stars 55 forks source link

Capture parser errors using xmlCtxtGetLastError #53

Closed the4thamigo-uk closed 6 years ago

the4thamigo-uk commented 6 years ago

fixes #52

lestrrat commented 6 years ago

I have a feeling you need to release the *xmlError struct somewhere?

the4thamigo-uk commented 6 years ago

Ah ok, but do you also have this problem then in XMLC14NDocDumpMemory()

lestrrat commented 6 years ago

Also, I agree that having this is better than not, but can you please note in the comment somewhere that in the future, we should probably be formatting the error message in C land, and not in Go (and therefore use xmlParserError or some other function that I may have overlooked).

If we can just fetch the error message from C, and worry about that alone, I think it's much better than mucking with C structs (i.e. xmlError) in Go land

lestrrat commented 6 years ago

Ah ok, but do you also have this problem then in XMLC14NDocDumpMemory

Well, then you might have found another memory bug :)

the4thamigo-uk commented 6 years ago

Not sure though, because no copy is made when returning, so I think it is correct not to free it :

https://github.com/GNOME/libxml2/blob/35e83488505d501864826125cfe6a7950d6cba78/error.c#L924

lestrrat commented 6 years ago

Oh interesting. Thanks for the pointer! Yeah, that looks like it belongs in the context. Can you keep a note of this in the comment too? I'm sure I'm going to scratch my head in the future on the same line :D

the4thamigo-uk commented 6 years ago

The remaining concern I have is whether we should reconstruct the error message formatted with the line number etc as you would see it in std out... or whether we should wrap the error structure so that the library user can format the error how they want (the more complete fix) .

the4thamigo-uk commented 6 years ago

Updated the commit comment as requested.

lestrrat commented 6 years ago

Well, I think for the time being this should suffice:

If I were the end user, I think I'd expect the GetLastError function to return a wrapper (or some copy) of the underlying *xmlError structure. So I think that function should return a struct. But the parser function can return whatever, really.

the4thamigo-uk commented 6 years ago

Yup ok, will do

the4thamigo-uk commented 6 years ago

Have updated the commit.

P.S. the error message 'failed to document' in the existing code doesnt mean a lot to me. How are you meant to interpret that as a user of the library?

lestrrat commented 6 years ago

That's probably one of those cut n paste (then a mass replace) things that got left over. Feel free to fix that in this PR or send me an issue so I don't forget :)

the4thamigo-uk commented 6 years ago

Oh shall I move the xmlCtxtLastError function to be near to ReportErrors?

lestrrat commented 6 years ago

You mean physical placement? I really have no preference there. Your call.

lestrrat commented 6 years ago

Well, I have to go take care of the kids now. Let me know when you finish the PR, and I will merge later. Thanks for your contribution!

the4thamigo-uk commented 6 years ago

Ok I moved it. Think its done, so long as it passes checks.