Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use sourcefile instead of 'just' headers #67

Closed
MCWertGaming opened this issue Feb 8, 2021 · 11 comments
Closed

use sourcefile instead of 'just' headers #67

MCWertGaming opened this issue Feb 8, 2021 · 11 comments

Comments

@MCWertGaming
Copy link
Collaborator

Hello,

i want to ask, if I can split the header files of this library into header and source files. Yes, I am aware of the desciption advertising this library as a "header only library", but I see many disadvantages here.

First of all we face many bad practices:

  • function bodies inside classes
  • internal enums and macros are exposed / included
  • includes get included into all projects (that mean users can't keep on track, wich libraries are inlcuded with the two headers(with include themselves as well) so all includes are also available inside the main project)
  • There are probably some more...

And I want to note, that this library is quiet small and consist of two headers. I would like to move the source parts into a source file and keep the class and function definitions inside of the header file (we can even move both header together, something you were / are already planing to do, if I saw that right). As most people (hopefully) use cpp-terminal as cmake dependency, there wouldn't be anything different for them at all, if they just copy the files they would have to copy two or 3/4 files and add the source file/s to their compile target - I wouldn't call that much more complicated.

Critizism is appreceated, it's an Idea - I hope this is not against the goal of this library!

Best regards
Damon

@certik
Copy link
Collaborator

certik commented Feb 8, 2021

Yeah, it's the usual issue of distributing C++ libraries. For LFortran I simply copy cpp-terminal into a tpl directory. So splitting into source/header files would not be a problem, I would just add the source file into LFortran's CMake file. So I am fine with that.

@wolfv, @SylvainCorlay what do you think is the best approach here?

@MCWertGaming
Copy link
Collaborator Author

I have looked into it and do you want to merge both terminal_base and terminal into one file? I think it would be better to do two seperate source files with seperate headers. I would even do a seperate header and source file to provide things like widgets and such.
My Idea would be:

  • terminal-base.hpp to provide the bare minimum to get you started with controling the terminal (that would include save_screen(), get_corsor_position(), the coloring and functions like that)
  • terminal.hpp to provide a class with a developer friendly kind of abstraction that provides a small and simple set of functions which do most of the work in the background so the user don't needs to deal with that (like ncurses does).
  • widgets.hpp / utils.hpp / extra.hpp (or however we will call it) for providing widgets to be used with terminal.hpp with things like progress bar, "windows" around text etc.
  • optionally we can also provide a small header for optional things that are not associated with terminal but quiet usefull for writing application, I'm thinking about things like a sleep function and things like that

What do you think?

@MCWertGaming
Copy link
Collaborator Author

@certik How do you would like to proceed with the header files?

terminal.cpp
terminal.h <- actual features
terminal_base.cpp
terminal_base.h <- platform dependent code

or

base.cpp
base.h <- colors, styles, print functions etc
extra.cpp
extra.h <- auto color, widgets, shapes etc (also prompt, but could be placed in it's own header file)
input.cpp
input.h <- input stuff
window.cpp
window.h <- window class

platform.h <- plattform specific code to keep the other files clean. Just using a internal header files allows to make all functions inline

@certik
Copy link
Collaborator

certik commented Oct 20, 2021

I would go with your second approach.

Regarding the platform dependent stuff, the way to do it is to have platform.h which contains the API (in one way or another) that by itself is platform independent, and then platform.cpp contains the implementation which contains #ifdef for each platform. That way all the platform dependent stuff is in just one file, platform.cpp, no other file and no header file. So every header file and every other cpp file is platform independent.

@MCWertGaming
Copy link
Collaborator Author

I was thinking about using the platform.h file as "Private" header -> project only. So we can simply create the smallest possible functions in the header file and include it into the source files of our project. The public headers would keep clean as well as the other source files. It would allow us to have the minimum for the platform independence inside of the platform header and keep all the logic and things that might need to be changed for new features in their source files.

Example:

platfrom.hpp

inline void get_term_size(row, col) {
  // logic with platform dependence
}
inline int get_input_raw() {
  // platform dependent parts
}

base.cpp:

#include "platform.hpp"
void Term::get_term_size(row, col) {
   get_term_size(row,col)
}
int get_input() {
  // parse get_input_raw for specific terminals
}

I wouldn't chose an approach to make the platform dependent stuff available directly, because those things are parts of others like the whole terminal size and interactive tty functions are part of the base.h header. Also not making the functions inline creates overhead on MSVC because it's unrolling only some functions while GCC and CLang are unrolling most things starting with -O2 (Have tested that with a friend a few weeks before out of interest). Also I'm not sure if it would bring any difference for coding here.
So what do you say @certik? Making a source file for platform as well or just a header? In the ends it's probably not that big of a deal (also the windows terminal is slow anyway. A hello world compiled with MSVC (and /Ofast) is already over 450 lines of assembly, while linux (gcc, also with -Ofast) needs 20 lines of assembly).

@MCWertGaming
Copy link
Collaborator Author

So what do you say?

@MCWertGaming
Copy link
Collaborator Author

@certik

@certik
Copy link
Collaborator

certik commented Oct 23, 2021

Either is fine with me. Yes, the header would be private. You can start with just a header and only include it in cpp files. If we need more, we can expand it.

@MCWertGaming
Copy link
Collaborator Author

Ok, I'll make a PR in the coming days. Thanks!

@certik
Copy link
Collaborator

certik commented Oct 23, 2021

Awesome, thanks!

@MCWertGaming
Copy link
Collaborator Author

Fixed in #132.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants