Merge of LFortran cpp-terminal changes

[dpoerio]

Manual merge of current cpp-terminal on LFortran and the main repo version. I confirmed this compiles in LFortran and things generally seem to work but the REPL test fails (runtime_error: tcgetattr() failed is thrown). Perhaps some testing of the examples here will reveal the problem.

[MCWertGaming]

You have merged it into an old commit of cpp terminal and are reverting some modernisation and clean up I did earlier. That should probably be the first thing to resolve. Have you just made modifications in terminal.h and base_terminal.h?
Then the examples probably wont work partly due to some reverts of my modifications.

[dpoerio]

I don't think it is so much that it was merged into an old commit (it was merged into the current master, at least from when I initially cloned the repo), but instead that certain things had simply diverged and due to conflicts it was not possible to always to accept the change from the most recent version. The initial multiline functionality was partially implemented into a older version, so in that sense perhaps there are still some pieces of older commits that are dated. When I have a moment I can also try a slightly different merge, being more sure to accept differences from the current master anywhere possible, if that should be considered the preferred version anywhere there are direct conflicts.

[MCWertGaming]

Yeah. I have done that already. I'm trying to resolve the errors an am done soon. Just let me ask one question: How does the multi line editing work? I have the prompt example working and it's doing well so far. I just don't get how I write further on the next line. Would be great if you could explain that quickly.

[MCWertGaming]

Also: would you prefer me to create a pull-request into your branch first or directly into master when i'm done. We would also need to port the changes into Window24 as it's the Window class but with 24bit (true color) instead of 4bit colors.

[dpoerio]

I just added in the changed prompt functionality, sorry about missing that. Everything should be a bit more clear now. The multiline prompt is now compiling and working from my branch. We added in some additional functionality for auto line continuation via a callback function to determine statement completeness. For the example I just have that always returning true.

Feel free to do the PR however you prefer, into my branch or master. Probably easier to just make a new PR in master if you've already resolved conflicts and have everything working.

[MCWertGaming]

I have reverted all changes that are caused by your old terminal.h file. All examples are working now again for me. One thing is that the prompt function has not changed. maybe it's a good way to merge it into the new implementation or remove it. Also the prompt example is not working for me. I have the compile error

[build] [8/9  88% :: 0.519] Building CXX object examples/CMakeFiles/prompt.dir/prompt.cpp.o
[build] FAILED: examples/CMakeFiles/prompt.dir/prompt.cpp.o 
[build] /bin/g++  -I../ -I. -g -fPIE -Wall -Wextra -pedantic -Werror -Wno-sign-compare -std=gnu++11 -MD -MT examples/CMakeFiles/prompt.dir/prompt.cpp.o -MF examples/CMakeFiles/prompt.dir/prompt.cpp.o.d -o examples/CMakeFiles/prompt.dir/prompt.cpp.o -c ../examples/prompt.cpp
[build] ../examples/prompt.cpp: In function 'int main()':
[build] ../examples/prompt.cpp:34:55: error: conversion from 'bool()' to non-scalar type 'std::function<bool(std::__cxx11::basic_string<char>)>' requested
[build]    34 |         std::function<bool(std::string)> iscomplete = determine_completeness;
[build]       |                                                       ^~~~~~~~~~~~~~~~~~~~~~
[build] ninja: build stopped: subcommand failed.
[build] Build finished with exit code 1

Could you please resolve that issue?
Also how are you testing the example? I mean it's not working for me or the CI (even though the CI is complaining about some more stuff I fixed separately).

[certik]

@MCWertGaming here is a demo how it is supposed to look like:

https://twitter.com/lfortranorg/status/1367507839535640578

Just press Alt-Enter or Ctrl-N to create a new line and you can use arrow keys to go up and down.

[dpoerio]

@MCWertGaming I can't replicate your error, and my build is failing before that. If you've fixed the other issues, can you push a branch with your changes, or make a PR into my branch? That way we'll know we're looking at the same thing.

Can you compile prompt.cpp by itself, with, i.e., g++ prompt.cpp -I..? This is working for me with my current multi2 branch.

[MCWertGaming]

@dpoerio Sorry my bad. I was trying to resolve a compiler warning because:

bool determine_completeness(std::string command) // <--
{
    // Determine if the statement is complete
    // Get the tokens
    bool complete;
    complete = true;
    return complete;
}

and command is an unused variable but when I remove that the problems goes all the way down to the implementation inside of terminal.h. What is the purpose of that variable? -Wall enables this error and it's enabled by default for the project using cmake.

I have checked all other things and they are working fine. I'll start a pull-request after we have resolved that problem.

[MCWertGaming]

Okay i found one issue. Using ALT+ENTER and CTRL+N works perfectly fine, but ALT+N causes a runtime error.

I have already looked into the code but ALT+N just falls through the switch so i'm not yet sure how that makes sense.

[dpoerio]

@MCWertGaming In general, the determine_completeness function is application specific to support auto-line continuation. The user would have to implement the logic in this function to determine if the statement/command that has been entered is complete, or if a new line should be automatically created by falling through to the alt-enter case. Normally that would be done by some analysis of the input string command. Since that is application specific, here we just have a generic function that always assumes the statement is complete by returning true no matter what.

One workaround may be changing

bool determine_completeness(std::string command)

to

bool determine_completeness(std::string)

[MCWertGaming]

Okay thank you! I have just silenced the error with [[maybe-unused]]. I have made some bad experience with arguments that don't have actual variables. (best example is #101 for that.)

[certik]

You can look if the string ends with \ (as in Python) and if it does, it will not be complete. That way you get rid of the warning and show how to use it at the same time.

[dpoerio]

Just incorporated this change into this PR. @MCWertGaming Feel free to use the same in your PR.

[MCWertGaming]

I will do a rebase! I create a pull request later.

[MCWertGaming]

I have created a PR @dpoerio. I tested all examples and they are working again.

[dpoerio]

[[fallthrough]] is a C++17 attribute. cpp-terminal still builds for me when changing CMake to use C++17. Is that OK in the context of the rest of the project, or do we need a different plan?

[certik]

If possible, I would allow older compilers than C++17, but that can be tackled once all tests pass.

Ping me once all tests pass and I will test this and review.

[MCWertGaming]

Nervermind, the problem is that the compilation is failing. The funny thing though is that the ALT+N runtime error I already noted is causing this because the ALT macro ist:

#define ALT_KEY(k) (char)(((unsigned char)(k) + 0x80))

That means that if you are doing ALT_KEY("n") we have 'n' + 0x80 which is 110 + 128 = 238. So we are casting 238 into a char (which can only hold 0 to 127).
reference:
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-3-c4310?view=msvc-160

We have to either remove the /W4 MSVC compiler warning from cmake (that would mean that ALT+N stays broken, like some ALT combinations seem to be) or we are removing that short cut. Another possible way would be to merge #80 where I have removed the macro (which also requires to make a work around to the alt key, because inline functions are not static values).

An idea for the third option would be to include the ALT and CTRL keys inside of the Key struct and do:

case Key::alt + 'q':

I have already tried that and it works fine, also I think this is a cleaner way for the users anyway. Then we wouldn't have to remove the switch statement or do a work around on the key switch statement. The reason why I haven't done that already in a PR is that there is an issue somewhere deep inside of Terminal.h. I can check on that again, but I would probably need some help on this one if you are up for it.

I think that @certik want's to comment on this one as well.

[certik]

We have to fix the Alt-N issue. But that should be a separate PR. In this PR, let's do the minimal changes necessary to get the multi-line REPL from LFortran into cpp-terminal. Then let's worry about polishing things, such as Alt-N.

[MCWertGaming]

Sure. I'll create an issue on this one quickly and just remove the shortcut. I hope that this is the last fix that has to be done.

Then there will be only the C++17 issue left.

[MCWertGaming]

Okay my test branch #105 is now working on all checks. I have created another pr for that @dpoerio.

@certik you can start on reviewing the code then. Just keep in mind that moving around in kilo is not working but i have fixed that in #101. Again sorry for that, I still don't understand why my change back then broke it.

[dpoerio]

@MCWertGaming Thanks so much for your help on this. Pinging @certik , tests are passing.

[certik]

I think that these changes look good. Thank you for @dpoerio for porting it here and thank you @MCWertGaming for fixing it up! I think it can be merged and we can improve upon it with further PRs.

We'll then copy it back to LFortran so that we use exactly the same cpp-terminal.

[MCWertGaming]

I'm happy to help!