Event access

[TobiasWallner]

This merge allows you to access the potential members of an event without constructing a new one. This could save calls to malloc especially when you copy paste longer strings.

I changed the class Term::Event so that it acts more like a std::variant would but tried to make it so that it is also usable for c++11. So not only do you save on calls to malloc you also save memory in generall.

It is literally a drop in replacement but offers a lot more features and access methods to the stored events.

Furthermore I changed the parse function so that it accts more like a constructor that is encapsulated from the other other constructors.

[flagarde]

I will have a look but maybe just with std::move we could avoid reallocation

[TobiasWallner]

I will have a look but maybe just with std::move we could avoid reallocation

Yes, and my major point here is that in the old event. If I wanted to get the lenth of the string, I could only do so using the casting operator which creates a new std::string. But I do not want to get a new std::string, I only want to know its size() so I need access operators that do not create a new instance but just return me a pointer or reference.

[TobiasWallner]

and with this you have those, you can just use get_if_screen() and it returns you either a pointer to the contained screen object or a nullptr.

[TobiasWallner]

@flagarde may i get your help resolving this linker error:

It appears that some compilers have trouble linking the function parse_event in event.cpp wheras others don't. Generally my PR seems to get accepted by c++11 to c++20 so it should not be a language problem, I guess.

 /usr/bin/ld: ../cpp-terminal/platforms/libcpp-terminal-platforms.a(input.cpp.o): in function `Term::Platform::read_raw()':
/__w/cpp-terminal/cpp-terminal/cpp-terminal/platforms/input.cpp:150: undefined reference to `Term::parse_event(std::string&&)'
collect2: error: ld returned 1 exit status

--- event.hpp ---
namespace Term{
...
Event parse_event(std::string&& str);
}  // namespace Term

--- event.cpp ---
Term::Event Term::parse_event(std::string&& str){ 
...
}

However, I see that the event.cpp file gets compiled before input.cpp so the linker should be able to resolve that reference, right?

Any Ideas why the linker cannot resolve that function call in input.cpp?

[flagarde]

Don't know why it's happening. I still try to understand what you try to do with the PR

[TobiasWallner]

@flagarde

I still try to understand what you try to do with the PR

Ok, so in the old Term::Event there are only the following casts availiable to acces the contained event:

operator Term::Key() const;
operator Term::Screen() const;
operator Term::Cursor() const;
operator std::string() const;

which construct a new object of the contained type. But I do not want to construct a new object of the contained type. Also constructing a new type may allocate new memory, which I definitelly do not want if not necessary. The information (aka. the contained type) is already there. So all I want is a reference or pointer to the contained type kinda like the get_if_<type>() from std::variant. I know that std::variant is c++17 but creating a function or method get_if_<type>() should be possible in c++11.

This is why I created the following functions, which either give you a pointer to the contained type or a null pointer.

Key* get_if_key();
const Key* get_if_key() const;
Screen* get_if_screen();
const Screen* get_if_screen() const;
Cursor* get_if_cursor();
const Cursor* get_if_cursor() const;
std::string* get_if_copy_paste();
const std::string* get_if_copy_paste() const;

And additionally to the .type() method, to find out if it is that type you want, I created the boolean functions:

bool is_empty() const;
bool is_key() const;
bool is_screen() const;
bool is_cursor() const;
bool is_copy_paste() const;

Then I thought, that, because only ever one of those types is ever actually used by the class I can overlap them like a union does. However, the std::string has a destructor and a union is not memory save and cannot be used for that purpose since it is not trivially destructable. So what I did was allocating a buffer in the class with the maximal alignment and memory size of all the contained types.

alignas(MAX_ALIGN) uint8_t m_variant[MAX_SIZE];

And then I made sure that the contained type is always appropriatelly treated, constructed and destructed for the contained type.

Does that make sense so far?

[flagarde]

Yes it makes sense to me but I wonder if this is not possible : 1) Using union to save space 2) Use the move constructor of key etc to receive Event and transfer by move the content of Event

[TobiasWallner]

Yes it makes sense to me but I wonder if this is not possible :

1. Using union to save space

2. Use the move constructor of key etc to receive Event and transfer by move the content of Event

c++ will not allow you to use a union, because std::string is not trivially destructable and when the union goes out of scope, it does not know which destructor to call. That is why you normally use a std::variant and why I tried it the way I tried it.

But it seems that earlier gcc compilers have a lacking support for alignas and alignof directives

[flagarde]

Yes, That why I didn't made one but we could take the str out of the union to still save space or more ugly use a char[BIG_NUMBER] to put it in the union

[flagarde]

Yes, That why I didn't made one but we could take the str out of the union to still save space or more ugly use a char[BIG_NUMBER] to put it in the union

[TobiasWallner]

So I have one last try and if this won't work I have no idea what is wrong in the build prozess and why the linker cannot find the function parse_event(std::string&&) on some targets but can on other.

[flagarde]

It's very strange indeed. I don't understand neither

[TobiasWallner]

could it be that sometimes, maybe the file event.cpp gets compiled before platform/input.cpp?

[TobiasWallner]

@flagarde finally now it works. I have no Idea why some linkers had problems linking a function because other functions were inlined. But now all targets are successful.

However, 2 checks were cancelled? what does that mean?

• docs / 📚 Docs (pull_request)
• docs / deploy (pull_request)

[TobiasWallner]

Now also fixes issue #279 @flagarde How can I link a PR with an issue?

[flagarde]

@TobiasWallner I have made some PR too to fix this and the legacy terminal on windows; Let see how we can merge both

[TobiasWallner]

I think it will merge no problem. My solution is to create an empty event in case that the string to parse an event is empty. The check is in the parse_event() function. So I guess our solutions are in different files and won't interfere with one another.

[flagarde]

@TobiasWallner I mean today I did some change on this #279 together with other change to deal (still badly) with legacy terminal. this is my work. As you where concerned about performance I change some stufs and now

• Event is 36bytes (before 52bytes) using union
• Even have good move constructor and =
• Other changes (not related to performances but to improve legacy terminal handling + fix #279 + better Alt+Ctlr+Key handling)

We need to work on a Convention for the variable, at least the one in the API. ALL_FULL_CASES seems very a macro thing. Personally I like this convention because macros are EVIL and so full uppercase catch attention.

In term of performance I think what could be beneficial is to update Input to block and wake up only when OS tell him user as witten to buffer. In linux I think i could find how to do it but in windows I don't know how to do. I will not only be more efficient but it will fi some problems we have now because user tap randoms keys and the 10ms are not enough so the 2 keys are merged in one buffer

[TobiasWallner]

Yes that is good. I actually thought of some other way to save mallocs today (spontaneous thought), where your union + string solution would be superior.

We could also provide a method to get an event, that does not create an event, but writes the new event into an existing one. So consecutive copy paste events could use the same string if the user/programmer decides to, with read_event(Event& out) or something like that.

[TobiasWallner]

Maybe I can find a way to have an OS block/wakeup for windows keyboard inputs.

[flagarde]

Maybe I can find a way to have an OS block/wakeup for windows keyboard inputs.

This would be save a lot of CPU I think and will make the program faster

[flagarde]

have a look at #282 it is a bit more conservative then your but I think the performance are improved.

[TobiasWallner]

I am closing this pull request due to the progress in #282