InkBin Header

[JBenda]

Adding inkcpp version and endianness to InkBin header.

No STL support will come after the PoC finished.

[brwarner]

Looks good so far. I guess maybe Endianness should be read first though? Since that would affect the encoding of the version numbers (for when, someday in the future, we actually do the flip).

[brwarner]

It's probably better to avoid std::tuple all together for consistency with the rest of the code base. The only STL code I have right now (which is all excluded if you turn off that preprocessor flag) is purely in user-facing code. My intent was that if you were already using STL and you wanted a prettier, STL friendly interface, you could turn on all those functions.

Admittedly, my reason for making STL optional in the runtime is not very compelling. Basically, since the project is supposed to work with Unreal, I didn't want to have all these and related STL includes when Unreal's API goes out of its way to implement their own versions of all those classes.

[JBenda]

I see, sorry will chage it. It was a stupid idea anyway.

I would like to introduce a readNumber function to hide the endian handling.

Or would it be ok to rewrite the in memeory copy of the inkbin if the endian differs?

also we need a kind of logger to warn about it (so he can recompile it to avoid the overhead)

Is the inkcpp version number already in a header?

[brwarner]

I see, sorry will change it. It was a stupid idea anyway.

Eh. Sometimes I try random C++ features just for fun to see how they work. I've never really used tuple much before actually.

I would like to introduce a readNumber function to hide the endian handling. Or would it be ok to rewrite the in memeory copy of the inkbin if the endian differs?

In runner_impl.cpp there's a template function called read which handlers reading a data type from the binary block. I think what you'd want to do is put the endian handling in there. It's a member function of runner_impl so it has access to the story object which would contain the new header information. I think you'd just check if the current machine endianness differs from the file's and if so, flip the bytes. It reads types of varying sizes so this way we can handle all of them in one place.

also we need a kind of logger to warn about it (so he can recompile it to avoid the overhead)

I don't have a good runtime logger yet! That's something that should be added at some point.

Is the inkcpp version number already in a header?

No. There is really no inkcpp version at all right now. Maybe we can make a new header file and define it? The header is written in binary_emitter.cpp's output function. You'll see right now I just write out the Ink version that I got from the JSON.

[JBenda]

Thanks for the hints. The read function was a nice spot. It's also without STL :stuck_out_tongue_winking_eye:. Is there more to-do for this feature? At the moment I set up a big endian system to check this :)

[brwarner]

Thanks for the hints

Hey, no problem.

Is there more to-do for this feature?

It looks like right now you're just always writing out "SAME" into the file for the endian encoding. I'm not sure if it makes sense to write SAME or DIFFER into the file, since the compiler doesn't really know who is going to be reading it. I think it made more sense to have what you had before: Big or Small being the enum values. Then the runtime can just check if its endian setting matches the file and if not, swap.

I did some googling concerning checking the endianness. Looks like this function works:

// source https://stackoverflow.com/questions/1001307/detecting-endianness-programmatically-in-a-c-program
bool is_big_endian(void)
{
    union {
        uint32_t i;
        char c[4];
    } bint = {0x01020304};

    return bint.c[0] == 1; 
}

I think you also want the value of the endianness enum to be only a single byte, otherwise the byte order could be wrong if you're reading from a different endianness.

[JBenda]

This was the idea, the compiler write SAME, and when the endian differs the runner reads DIFFER, so i do not care about the actual ending.

So we don't realy care about the actual endian. But we can also use explicit big and small endian. I think it's just a matter of taste.

[brwarner]

Ohhhhhh I see now. You were just too clever for me. Awesome!

I might make a few code style tweaks for consistency, then I'll merge this in.

On Tue, Feb 2, 2021, 11:56 PM JBenda, notifications@github.com wrote:

This was the idea, the compiler write SAME, and when the endian differs the runner reads DIFFER, so i do not care about the actual ending.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/brwarner/inkcpp/pull/8#issuecomment-772311663, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIAEVFWWFSVMSQK6ASMKOLS5D6SHANCNFSM4W5TF7FQ .

[JBenda]

Ok, have you somewhere a written form of you're coding style ^^, than I can adapt, and you can spare time ^^;

[JBenda]

What should be the reaction when the inkcpp version dose not match ? An warning or an exception? It is still possible that a different Version will execute the programm correctly :thinking:

[brwarner]

What should be the reaction when the inkcpp version dose not match ? An warning or an exception? It is still possible that a different Version will execute the programm correctly 🤔

I think the reaction should be a runtime exception on load. My idea for the inkcpp version number is it should only increment when there are breaking changes to the format.

Ok, have you somewhere a written form of you're coding style ^^, than I can adapt, and you can spare time ^^;

I'll add a Contributing guide this weekend :)

[JBenda]

Cool :) :+1:

Then I will add an exception on load.

[brwarner]

Also, if you merge in the master branch you'll get the new GitHub Actions which will automatically test if your branch builds on each of the OSs.

[JBenda]

This sounds awesome :+1 !

But the platform is still not at the master, so I can't merge it there, because my branches based on the platform. Otherwise, I can't run them locale ^^.