Byte Order Mark (BOM) handling functions rewrite #3880

burner · 2015-12-18T10:30:44Z

move to std.encoding
less overengineering

JackStouffer · 2015-12-18T14:41:14Z

std/encoding.d

+    the found $(D BOM) or $(D BOM.invalid).
+*/
+BOM getBOM(Range)(Range input)
+        if(isInputRange!Range)


We need a standardized way of declaring template constraints, because this is the fourth unique variant in style that I have seen.

I'll bet one round of drinks, for everybody at DConf 2016, that that forum discussion is going to become a bikeshedding discussion!

Maybe we should just have Walter or Andrei make a decision, as it only effects Phobos maintainers and the differences in our options are trivial.

What about going with what dfmt does?

You know what, forget I said anything. This is a minor point and I don't want this to turn this into a bikesheading discussion.

dfmt is anti-bikeshedding

JackStouffer · 2015-12-18T14:42:06Z

The logic however looks good, much cleaner this time :)

schveiguy · 2015-12-18T16:38:21Z

std/encoding.d

+                        [0x2B, 0x2F, 0x76, 0x39],
+                        [0x2B, 0x2F, 0x76, 0x2B],
+                        [0x2B, 0x2F, 0x76, 0x2F],
+                        [0x2B, 0x2F, 0x76, 0x38, 0x2D]


I thought there was only 4 of these?

schveiguy · 2015-12-18T22:02:26Z

std/encoding.d

+/** Mapping of a byte sequence to $(B Byte Order Mark (BOM))
+*/
+enum bomEntries = [
+        tuple(BOM.utf32be, to!(ubyte[])([0x00, 0x00, 0xFE, 0xFF])),


Don't use enum for arrays, this will create a new array every time you use it. Instead, do immutable.

good to know

JakobOvrum · 2015-12-20T01:35:48Z

Is there any good reason for bomEntries and getBOMTableIndex to be public?

JakobOvrum · 2015-12-20T01:38:03Z

std/encoding.d

+        }
+    }
+
+    assert(false, "This can never happen, unless startsWith consider an empty"


startsWith matches the shortest of its needles, and an empty needle always matches.

yes, as I wrote. Unless startsWith changes this assert will never execute. If it does people will know about it.

burner · 2015-12-20T11:02:55Z

bomEntries and getBOMTableIndex are public so you use them to add a BOM to a file you created

JakobOvrum · 2015-12-20T12:58:06Z

How does getBOMTableIndex help with that?

burner · 2015-12-20T13:31:59Z

File -> getBOMTableIndex -> new File -> prepend same BOM sequence by using the index of getBOMTableIndex and bomEntries.

utf7 could have several start sequences, only using getBOM does not work

JakobOvrum · 2015-12-21T01:48:20Z

Well, like Steven suggested, getBOM should return the tuple.

Also, the name bomEntries doesn't make a lot of sense. Entries into what, exactly? Going by getBOMTableIndex it seems like bomTable.

burner · 2015-12-21T09:19:53Z

@JakobOvrum @schveiguy fixed

JakobOvrum · 2015-12-21T09:40:50Z

std/encoding.d

+    the found $(D Tuple) of $(D BOM) and the $(D BOM) sequence as a
+    $(D ubyte[]).
+*/
+auto getBOM(Range)(Range input)


Don't use return type inference when the return type is easy to represent and provides documentation value. The return type here is always Tuple!(BOM, immutable ubyte[]).

Named fields ala Tuple!(BOM, "bom", immutable ubyte[], "byteSequence") is probably also desirable.

JakobOvrum · 2015-12-21T13:09:54Z

std/encoding.d

+/** Mapping of a byte sequence to $(B Byte Order Mark (BOM))
+*/
+immutable bomTable = [
+        tuple(BOM.noBOM, new ubyte[0]),


Do we really need this in the table?

IMO yes, otherwise you would have to handle the absence of a BOM differently

getBOM can still return it

JakobOvrum · 2015-12-21T13:15:53Z

std.file.read returns void[]. Would it be an idea to accept const(void)[] inputs?

JakobOvrum · 2015-12-21T13:21:06Z

std/encoding.d

+    the found $(D Tuple!(BOM,ubyte[])) of $(D BOM) and the $(D BOM) sequence
+    as a $(D ubyte[]).
+*/
+auto getBOM(Range)(Range input)


Not sure why Github marked it as outdated, but my comment about explicit return type still stands

schveiguy · 2016-03-11T16:25:24Z

I just realized something -- nowhere do you specify a fully decoded BOM, e.g. dchar(0xFEFF), this would probably be good to include somehow.

burner · 2016-03-11T20:58:07Z

@schveiguy I do not follow, please eleborate what you mean by fully decoded BOM

schveiguy · 2016-03-13T13:14:36Z

@burner what I mean is that if you have string data (i.e. a dchar, or a utf dchar range), it is nice to have it defined, something like:

enum dchar utfBOM = 0xfeff;

So you can do stuff like:

// skip any BOM
if(myString.front == utfBOM) myString.popFront;

without having to remember the hex version.

burner · 2016-03-16T10:23:25Z

@schveiguy what about encodings whos bom does not fit into a dchar utf16(le,be) utf8?
I mean what you are describing is basically the use case of getBOM. It feels weird IMO that you have a string of know type and then ask for its BOM to find out its type? Maybe I'm not understand what you are trying to say.

schveiguy · 2016-03-16T15:02:46Z

The BOM is used to determine the encoding. But it's possible you already know the encoding. Then the BOM becomes another character. Having a definition of that somewhere is useful.

For instance, in my iopipe library, I have an example program "convert" that reads a file, and converts it to another encoding. In this section of code, I have determined the encoding and have the input already set up to process the data. However, on output, I need to ensure there is a BOM in the new file. So I have this code:

    if(!input.window.empty && input.window.front != 0xfeff)
    {
        // write a BOM if not present
        put(oChain, dchar(0xfeff));
    }

It would be nice to just have that constant be a manifest constant somewhere so that I don't have to remember the hex code.

DmitryOlshansky · 2016-04-03T08:53:22Z

@burner address the last nit - add BOM constant, after all BOM has a constant codepoint assigned to it. Then it should be ready for final nod of approval from @andralex

burner · 2016-04-06T08:09:21Z

@DmitryOlshansky the BOM constants will not work as not all BOM constants are 4 byte long. The presented use case is exactly what getBOM is for. To append a BOM I plan to create to createBOM(OutputRange) next.

DmitryOlshansky · 2016-04-06T09:21:18Z

@burner You miss the point. BOM is a codepoint, please add the constant stop worrying about encoding, you already covered byte-level representations.

burner · 2016-04-06T09:44:08Z

@DmitryOlshansky added the constant not sure about the doc though

schveiguy · 2016-04-07T15:29:30Z

Thanks for adding that, LGTM.

burner · 2016-04-26T08:58:00Z

ping @DmitryOlshansky @schveiguy @JakobOvrum @andralex

DmitryOlshansky · 2016-04-26T09:29:02Z

LGTM

andralex · 2016-04-26T17:43:15Z

std/encoding.d

+/** Mapping of a byte sequence to $(B Byte Order Mark (BOM))
+*/
+immutable bomTable = [
+    BOMSeq(BOM.none, new ubyte[0]),


Doesn't null work here?

andralex · 2016-04-26T17:47:19Z

I approve, please pull after nits. Thanks!

* move to std.encoding * less overengineering dlang#3870 rework Don't use top-level selective import in std.math because of DMD issue 314. some quickfur comments whitespace remove used import steven suggestion utfBom andrei nitpicks andrei null

burner · 2016-05-02T18:42:04Z

@DmitryOlshansky @schveiguy @JakobOvrum @andralex fixed the nitpicks

schveiguy · 2016-05-02T19:25:17Z

Auto-merge toggled on

burner · 2016-05-02T19:29:19Z

@schveiguy thanks

JackStouffer · 2016-05-02T19:43:40Z

std/encoding.d

@@ -3361,3 +3361,175 @@ version(unittest)
        return "0123456789ABCDEF"[n & 0xF];
    }
 }
+
+import std.typecons;


Why here and not at the top like everything else?

schveiguy · 2016-05-02T19:52:19Z

@JackStouffer we can fix that after if it's really a sticking point. The auto tester is almost done, and it takes 1 hour to run the current test :)

JackStouffer reviewed Dec 18, 2015
View reviewed changes

schveiguy reviewed Dec 18, 2015
View reviewed changes

burner force-pushed the bom2 branch from fc5793b to bb930ea Compare December 18, 2015 21:59

schveiguy reviewed Dec 18, 2015
View reviewed changes

burner force-pushed the bom2 branch 2 times, most recently from fd36fad to 7c886fe Compare December 19, 2015 10:37

JakobOvrum reviewed Dec 20, 2015
View reviewed changes

burner force-pushed the bom2 branch from 96874d3 to 57c0805 Compare December 21, 2015 09:20

JakobOvrum reviewed Dec 21, 2015
View reviewed changes

burner force-pushed the bom2 branch from 57c0805 to 2949f1f Compare December 21, 2015 12:46

JakobOvrum reviewed Dec 21, 2015
View reviewed changes

burner force-pushed the bom2 branch from 6ce05e3 to abdc77d Compare March 11, 2016 21:20

DmitryOlshansky added the @andralex label Apr 3, 2016

burner force-pushed the bom2 branch from abdc77d to 244a1d8 Compare April 6, 2016 09:43

burner force-pushed the bom2 branch from 244a1d8 to b39cb37 Compare April 26, 2016 08:57

andralex reviewed Apr 26, 2016
View reviewed changes

burner force-pushed the bom2 branch from b39cb37 to 67563b7 Compare May 2, 2016 18:37

burner force-pushed the bom2 branch from 67563b7 to 3d37aee Compare May 2, 2016 18:39

JackStouffer reviewed May 2, 2016
View reviewed changes

schveiguy merged commit 30587d4 into dlang:master May 2, 2016

burner mentioned this pull request May 2, 2016

move typecons import to top of encoding #4277

Merged

Byte Order Mark (BOM) handling functions rewrite #3880

Byte Order Mark (BOM) handling functions rewrite #3880

Conversation

burner commented Dec 18, 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

JackStouffer commented Dec 18, 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

JakobOvrum commented Dec 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

burner commented Dec 20, 2015

JakobOvrum commented Dec 20, 2015

burner commented Dec 20, 2015

JakobOvrum commented Dec 21, 2015

burner commented Dec 21, 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

JakobOvrum commented Dec 21, 2015

Choose a reason for hiding this comment

schveiguy commented Mar 11, 2016

burner commented Mar 11, 2016

schveiguy commented Mar 13, 2016

burner commented Mar 16, 2016

schveiguy commented Mar 16, 2016

DmitryOlshansky commented Apr 3, 2016

burner commented Apr 6, 2016

DmitryOlshansky commented Apr 6, 2016

burner commented Apr 6, 2016

schveiguy commented Apr 7, 2016

burner commented Apr 26, 2016

DmitryOlshansky commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andralex commented Apr 26, 2016

burner commented May 2, 2016

schveiguy commented May 2, 2016

burner commented May 2, 2016

Choose a reason for hiding this comment

schveiguy commented May 2, 2016