Improve Unicode related error messages #11573

ScottPJones · 2015-06-04T13:30:07Z

This improves the error messages for the Unicode support, in utf8.jl, utf16.jl, and utf32.jl
They will save the position in the string where the error occurred, as well as the character or code unit that caused the error, and the show method for the UnicodeError exception type displays that extra information.

yuyichao · 2015-06-04T13:32:20Z

base/utf16.jl

@@ -39,7 +29,7 @@ function next(s::UTF16String, i::Int)
    elseif length(s.data)-1 > i && utf16_is_lead(s.data[i]) && utf16_is_trail(s.data[i+1])
        return utf16_get_supplementary(s.data[i], s.data[i+1]), i+2
    end
-    throw(ArgumentError("invalid UTF-16 character index"))
+    throw(UnicodeError(UTF_ERR_INVALID_INDEX,0,0))


I guess the GC frame concern still apply here.

@yuyichao Please get that fixed! 😀

I've actually got a new idea this morning and would like to try out. (Sth like a throw local noinline). Maybe that can bring down the gap between #11284 and #11508 .

tkelman · 2015-06-04T13:34:17Z

base/sysimg.jl

@@ -84,6 +84,8 @@ include("iterator.jl")
 include("osutils.jl")

 # strings & printing
+include("utferror.jl")
+include("utftype.jl")


totally superficial, sorry... I think this would be a tiny bit better as utftypes.jl ?

p.s. thank you thank you thank you for opening this separately. 👍

tkelman · 2015-06-04T13:56:41Z

Would be nice to test at least a few of these error paths if we can do that yet. Address the comments inline and I'd say this is an easy merge, thanks!

tkelman · 2015-06-04T14:00:59Z

base/utftype.jl

+UTF32String(data::Vector{UInt32}) = UTF32String(reinterpret(Char, data))
+
+const empty_utf16 = UTF16String(UInt16[0])
+const empty_utf32 = UTF32String(UInt32[0])


Can these wait to be added until the commit that first uses them?

ScottPJones · 2015-06-04T14:45:19Z

The full testing really happens when you have utfcheck.jl, which I'm getting ready right now, but I did add some more in test/unicode.jl

tkelman · 2015-06-04T14:50:56Z

The full testing really happens when you have utfcheck.jl, which I'm getting ready right now, but I did add some more in test/unicode.jl

Yes, that makes sense that the coverage of this will go way up with the more extensive changes that will come separately. Tests of the error paths would've caught the typos like having UTF_ERR_ODD_32 instead of UTF_ERR_ODD_BYTES_32. Push any updates as they're ready.

Would be nice to make your commit messages a bit more descriptive too, if possible.

tkelman · 2015-06-04T15:06:14Z

base/utferror.jl

@@ -0,0 +1,30 @@
+# This file is a part of Julia. License is MIT: http://julialang.org/license
+
+##\brief      Error messages for Unicode / UTF support


I see what you did there. At least the backslash is less confusing here than @. @jakebolewski what do you think, can we live with this? Or merge anyway and delete it afterwards if we really don't like the doxygen-style comments?

Yes, I see the point about looking like a macro... that was done because in Python, for a docstring, you must use @ instead of \, because \ is an escape character inside a string.
Maybe for Julia, to put doxygen information into doc strings, we'd need to use a different start character, to avoid the confusion with macros, and then convert it to @ before passing them to
the doxygen processor.
The only difference now between "plain text" and working doxygen (it actually is the valid Python doxygen style), is the added \. Whether or not doxygen use becomes prevalent (see #11123) in the rest of julia, that information is very useful, so I'd hope you'd accept having a few extra \ characters...
If nobody starts documenting the code in Base, it never is going to happen!

Comments are awesome. Extra annotations in the comments that don't do anything yet and don't mean anything to the majority of us who've probably never used doxygen are just noise.

Sorry, but to me, they help a lot when working on the code, even without the very nice stuff that doxygen builds from them... (have you ever tried doxygen on a very large project? It really is helpful...)

Okay, but you're contributing to a pre-existing large project that doesn't use doxygen (yet, maybe that will change but that's not for this PR to worry about), and at least 2 core contributors have told you to knock it off with the doxygen annotations.

@one-more-minute You've convinced me... (as a matter of fact, I've worked for a while doing just that, pulling information out of unstructured data... I should have thought of that!).
However, don't you need at least some standardization of tags, so you could pick out which part documents the parameters, which part the return value, which part the errors thrown (if any)?
For example: I've seen ### Arguments:, ### Returns:, Inputs:, Outputs:, ### Arguments, ### Keyword Arguments, etc... as you see, no consistency...
It doesn't even have to be complete consistency, it can be a many-to-one mapping... fairly free form, as long as there aren't too many options for the metadata parser to figure out, or for people to understand.

would at least give other Julia contributors time to evaluate the format, good or bad.

Well I've been evaluating by spending more time than I'd like reviewing your PR's, and so far I don't like the doxygen markup. But I don't care enough to let it block this PR which is otherwise worth merging, so I'm just going to do that now.

And I was just going to change this to using something more along the lines of Mike's idea... 😀 I guess I can do that in a later PR, as soon as the dust as settled a bit on the parsing Markdown ideas... Thanks! (and thanks for taking the time, and pushing back on the doxygen format, because now I will push for something better for documentation, which will still allow me to generate output for doxygen, but which fits better with Julia, and which I hope you do like)

However, don't you need at least some standardization of tags

Sure, I think it's completely reasonable to settle on some conventions eventually (these things usually happen naturally anyway). I think @catawbasam is right about side issues, though, so let's keep that discussion focussed in #8966 so that things can move forward here.

@tkelman and I've been spending more time than I'd like changing things so frequently 😀 but nobody has been holding a gun to either of our heads to make you review, or me to respond to your reviews. A long process yes, and I'm sorry for the time it has taken, but I think in the end, the final result for julia is much better. Thanks again for your time.

tkelman · 2015-06-04T18:20:10Z

Alright aside from the doxygen nits I think we can merge this one anyway.

Anyone have objections or additional feedback?

tkelman · 2015-06-04T18:39:30Z

By the way, reading through my old emails, here's one pretty easy way of making multi-line commit messages from the command line:

git commit -m 'The first line which should be <50 chars

^ be sure to put that first extra newline there, by convention
then go nuts, as descriptive as you like, until you are done
at which point you close the quote'

Improve Unicode related error messages

tkelman added the domain:unicode Related to unicode characters and encodings label Jun 4, 2015

yuyichao reviewed Jun 4, 2015
View reviewed changes

tkelman added the domain:error handling Handling of exceptions by Julia or the user label Jun 4, 2015

ScottPJones mentioned this pull request Jun 4, 2015

Fix #10959 bugs with UTF-16 conversions #11551

Merged

tkelman reviewed Jun 4, 2015
View reviewed changes

Improve error messages

0b158a6

ScottPJones force-pushed the spj/utferror branch from 544801b to 0b158a6 Compare June 4, 2015 14:57

tkelman reviewed Jun 4, 2015
View reviewed changes

tkelman mentioned this pull request Jun 4, 2015

doxygen + markdown to document C/C++ code #11123

Closed

ScottPJones mentioned this pull request Jun 4, 2015

Add UTF encoding validity functions #11575

Merged

tkelman added a commit that referenced this pull request Jun 5, 2015

Merge pull request #11573 from ScottPJones/spj/utferror

47a0eb1

Improve Unicode related error messages

tkelman merged commit 47a0eb1 into JuliaLang:master Jun 5, 2015

ScottPJones deleted the spj/utferror branch June 5, 2015 11:02

tkelman mentioned this pull request Jun 5, 2015

throw ArgumentError when accessing an invalid UTF-8 indice, rather th… #11423

Merged

tkelman mentioned this pull request Jul 29, 2015

Make show(io, ::UnicodeError) output start with UnicodeError: #12371

Merged

Sacha0 mentioned this pull request Oct 15, 2017

similar and traits #18161

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Unicode related error messages #11573

Improve Unicode related error messages #11573

ScottPJones commented Jun 4, 2015

yuyichao Jun 4, 2015

ScottPJones Jun 4, 2015

yuyichao Jun 4, 2015

tkelman Jun 4, 2015

tkelman commented Jun 4, 2015

tkelman Jun 4, 2015

ScottPJones commented Jun 4, 2015

tkelman commented Jun 4, 2015

tkelman Jun 4, 2015

ScottPJones Jun 4, 2015

tkelman Jun 4, 2015

ScottPJones Jun 4, 2015

tkelman Jun 4, 2015

ScottPJones Jun 5, 2015

tkelman Jun 5, 2015

ScottPJones Jun 5, 2015

MikeInnes Jun 5, 2015

ScottPJones Jun 5, 2015

tkelman commented Jun 4, 2015

tkelman commented Jun 4, 2015

		@@ -0,0 +1,30 @@
		# This file is a part of Julia. License is MIT: http://julialang.org/license

		##\brief Error messages for Unicode / UTF support

Improve Unicode related error messages #11573

Improve Unicode related error messages #11573

Conversation

ScottPJones commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jun 4, 2015

Choose a reason for hiding this comment

ScottPJones commented Jun 4, 2015

tkelman commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Jun 4, 2015

tkelman commented Jun 4, 2015