InkBin Header #8

JBenda · 2021-02-01T22:50:26Z

Adding inkcpp version and endianness to InkBin header.

No STL support will come after the PoC finished.

brwarner · 2021-02-01T23:04:43Z

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).

The memory layout of std::tuple is not intuitive and not defined from the standard. There for we will use an explicit read. Check out this example: the output for me was: `42 : 10` where I expect: `10 : 42` compile flags: `--std=c++20` compiler: `g++ 10.2.0` ```cpp struct Foo { static Foo create(const char * data) { Foo foo; foo._data = *reinterpret_cast<const decltype(_data)*>(data); return foo; } int& x = std::get<0>(_data); int& y = std::get<1>(_data); std::tuple<int,int> _data; }; int main(int argc, char *argv[]) { int buffer[] = {10, 42}; auto f = Foo::create((char*)buffer); std::cout << f.x << " : " << f.y << std::endl; return 0; } ```

brwarner · 2021-02-02T03:11:14Z

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 · 2021-02-02T09:26:37Z

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 · 2021-02-02T17:12:26Z

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 · 2021-02-02T23:30:06Z

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

brwarner · 2021-02-03T01:11:47Z

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 · 2021-02-03T07:56:04Z

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 · 2021-02-03T16:46:29Z

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, ***@***.***> 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 <#8 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIAEVFWWFSVMSQK6ASMKOLS5D6SHANCNFSM4W5TF7FQ> .

JBenda · 2021-02-04T14:47:30Z

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

JBenda · 2021-02-04T15:01:12Z

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 🤔

brwarner · 2021-02-04T16:35:55Z

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 · 2021-02-04T19:48:02Z

Cool :) 👍

Then I will add an exception on load.

brwarner

Added the code style guidelines as I mentioned. Mostly it's just casing. I added comments on everything that should change. Hopefully it's not too pedantic lol. I just like a unified naming scheme, whatever scheme it happens to be.

I liked to the corresponding section in each comment, but the full guide is here.

https://github.com/brwarner/inkcpp/blob/master/CONTRIBUTING.md

brwarner · 2021-02-05T21:12:39Z

shared/private/header.h

+
+#include "system.h"
+
+namespace ink {


Should be ink::internal since this is not a user facing type.

https://github.com/brwarner/inkcpp/blob/master/CONTRIBUTING.md#namespaces

brwarner · 2021-02-05T21:13:38Z

shared/private/header.h

+#include "system.h"
+
+namespace ink {
+		struct Header {


Should be in snake_case (i.e. header)

https://github.com/brwarner/inkcpp/blob/master/CONTRIBUTING.md#code-style

brwarner · 2021-02-05T21:19:36Z

shared/private/header.h

+				DIFFER = 0x0100
+			} endien = ENDENSE::NONE;
+			uint32_t inkVersionNumber = 0;
+			uint32_t inkCppVersionNumber = 0;


Enum type, values, and struct members should be in snake_case.

https://github.com/brwarner/inkcpp/blob/master/CONTRIBUTING.md#code-style

brwarner · 2021-02-05T21:23:12Z

shared/public/version.h

+#include "system.h"
+
+namespace ink {
+	constexpr uint32_t VERSION = 0;


Compile-time constants should be in PascalCase.

Also maybe name this something a bit more specific like InkBinVersion perhaps?

https://github.com/brwarner/inkcpp/blob/master/CONTRIBUTING.md#code-style

brwarner · 2021-02-05T21:24:41Z

inkcpp/story_impl.cpp


 #ifdef INK_ENABLE_STL
 #include <iostream>
 #endif

+namespace ink {
+	// FIXME: find appropriate place to define
+	Header Header::parse_header(const char *data)


Maybe in a new header.cpp file located in the inkcpp folder. You can add new cpp files to CMake by adding them to the CMakeLists.txt file in that folder. You'll see a giant list of source files there. Since it's only used by the runtime, the code can live in that folder.

brwarner · 2021-02-05T21:25:27Z

inkcpp/story_impl.cpp


 #ifdef INK_ENABLE_STL
 #include <iostream>
 #endif

+namespace ink {


Since this is a struct internal, it should live in ink::internal

brwarner · 2021-02-05T21:27:16Z

inkcpp_compiler/emitter.h

@@ -2,6 +2,7 @@

 #include "command.h"
 #include "system.h"
+#include "version.h"


Looks like you don't actually use this header here? You can just include it in the relevant cpp file.

brwarner · 2021-02-05T21:36:37Z

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 · 2021-02-07T14:00:22Z

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 ^^.

brwarner

Looks great!

InkBin Header (#8)

Initial thoughts

144ce57

JBenda added 4 commits February 2, 2021 23:19

Simpler header

925a190

Add byte swap to runner_impl::read

8634712

Add version and header writing

ca3632b

Fix struct padding

2992069

JBenda changed the base branch from master to platform February 4, 2021 14:46

JBenda marked this pull request as ready for review February 4, 2021 14:47

JBenda changed the title ~~InkBin Header [WIP]~~ InkBin Header Feb 4, 2021

Add exception when inkCppVersion won't match

e6e5b4f

brwarner requested changes Feb 5, 2021

View reviewed changes

Adapt to coding conventions

1a976a5

JBenda requested a review from brwarner February 7, 2021 14:00

brwarner approved these changes Feb 7, 2021

View reviewed changes

brwarner merged commit b1d6085 into JBenda:platform Feb 7, 2021

brwarner added a commit that referenced this pull request Feb 7, 2021

Merge pull request #13 from brwarner/platform

55674d7

InkBin Header (#8)

JBenda deleted the feature/InkBinHeader branch February 7, 2021 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InkBin Header #8

InkBin Header #8

JBenda commented Feb 1, 2021 •

edited

Loading

brwarner commented Feb 1, 2021

brwarner commented Feb 2, 2021 •

edited

Loading

JBenda commented Feb 2, 2021 •

edited

Loading

brwarner commented Feb 2, 2021

JBenda commented Feb 2, 2021

brwarner commented Feb 3, 2021 •

edited

Loading

JBenda commented Feb 3, 2021 •

edited

Loading

brwarner commented Feb 3, 2021 via email

JBenda commented Feb 4, 2021

JBenda commented Feb 4, 2021

brwarner commented Feb 4, 2021

JBenda commented Feb 4, 2021

brwarner left a comment

brwarner Feb 5, 2021

brwarner Feb 5, 2021

brwarner Feb 5, 2021

brwarner Feb 5, 2021

brwarner Feb 5, 2021

brwarner Feb 5, 2021

brwarner Feb 5, 2021

brwarner commented Feb 5, 2021

JBenda commented Feb 7, 2021

brwarner left a comment

InkBin Header #8

InkBin Header #8

Conversation

JBenda commented Feb 1, 2021 • edited Loading

brwarner commented Feb 1, 2021

brwarner commented Feb 2, 2021 • edited Loading

JBenda commented Feb 2, 2021 • edited Loading

brwarner commented Feb 2, 2021

JBenda commented Feb 2, 2021

brwarner commented Feb 3, 2021 • edited Loading

JBenda commented Feb 3, 2021 • edited Loading

brwarner commented Feb 3, 2021 via email

JBenda commented Feb 4, 2021

JBenda commented Feb 4, 2021

brwarner commented Feb 4, 2021

JBenda commented Feb 4, 2021

brwarner left a comment

Choose a reason for hiding this comment

brwarner Feb 5, 2021

Choose a reason for hiding this comment

brwarner Feb 5, 2021

Choose a reason for hiding this comment

brwarner Feb 5, 2021

Choose a reason for hiding this comment

brwarner Feb 5, 2021

Choose a reason for hiding this comment

brwarner Feb 5, 2021

Choose a reason for hiding this comment

brwarner Feb 5, 2021

Choose a reason for hiding this comment

brwarner Feb 5, 2021

Choose a reason for hiding this comment

brwarner commented Feb 5, 2021

JBenda commented Feb 7, 2021

brwarner left a comment

Choose a reason for hiding this comment

JBenda commented Feb 1, 2021 •

edited

Loading

brwarner commented Feb 2, 2021 •

edited

Loading

JBenda commented Feb 2, 2021 •

edited

Loading

brwarner commented Feb 3, 2021 •

edited

Loading

JBenda commented Feb 3, 2021 •

edited

Loading