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

Returning to the project, new todo list #33

Open
3 of 7 tasks
edhebi opened this issue Mar 28, 2020 · 8 comments
Open
3 of 7 tasks

Returning to the project, new todo list #33

edhebi opened this issue Mar 28, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@edhebi
Copy link
Owner

edhebi commented Mar 28, 2020

Haven't been active on this project for a while, so here are a few things to keep me occupied for a bit, in no particular order:

  • Fix the Event::EventFilter mess. It's overengineered, messy, and most definetly out of scope. Removal might be the best thing to do here.

  • Add tests. I'm still not sure how to format them, but it'd be good to have them. This includes unit tests, static analysis, more warnings, sanitizers, whatever makes sence for us.

  • Have a root cmake file to automate stuff. Three reasons:

    1. Have a nicer way to build the doc, run formatter, build examples, run the future tests, run clang format, and so on...
    2. Have a fixed set of warnings and options everywhere, instead of relying on whatever is defaults on different compilers and IDE.
    3. Even if it feels a bit pointless for a header only library, the community seems to go towards cmake everywhere to automate stuff as much as possible, and I kinda agree with that.
  • Should we go as far as setting up a package manager ? I realy like conan, but I'm not sure of the gain.

  • Doxygen is ugly af, and definetly not a pleasure to navigate. That being said, I don't know enough about the other options to have a real opinion. Let's look at what the community does.

  • Maybe not do everything in a single master branch ? It's not a problem now, but it's not ideal either.

  • Should we support c++ <17 ? This seems to imply macros and preprocessor to still take advantage of recent features when available, and I don't like the sound of it.

Those points are open to debate and some need conversation. Feel free to butt in and say stuff (looking at you, @Ybalrid :p). To me, those are the things to fix before a real 1.0.0 release tag.

@edhebi edhebi added the enhancement New feature or request label Mar 28, 2020
@edhebi edhebi added this to the First release milestone Mar 28, 2020
@edhebi edhebi self-assigned this Mar 28, 2020
@Ybalrid
Copy link
Collaborator

Ybalrid commented Mar 28, 2020

So, my brain is mashed potato right now (did not sleep yet, just finished Half-Life: Alyx, I'm processing what happened yet because OMFG)

Point by point, here's what I think:

  • I agree about the event filter thing. Kill it with fire.
  • Tests are good.
  • I'm think we should revisit that CMake thing, at least for the CI build and stuff
  • I only had bad experiences with C++ package manager. I don't think that's necessary for a folder of header files. If a user cannot -I some_folder or whatever equivalent, it's not really my problem IMHO :upside_down:
  • Doxygen can be styled, and some navigation options can be activated that may make it feel "nicer"
  • master = release, but since we never released... We should have a clean master branch, a develop one where "done" features are merged together, and specific pieces of work are advanced on feature branches that are to be merged to develop. Once something is ready to be stabilized and released, it's merged to master. Once happy we tag a specific commit and voilà.
  • Why supporting C++ < 17? I see no practical reasons not to.

@edhebi
Copy link
Owner Author

edhebi commented Mar 28, 2020

Why supporting C++ < 17? I see no practical reasons not to.

do you mean "I see no reason to support C++14", or "I see no reason not to support C++14" ?

@Ybalrid
Copy link
Collaborator

Ybalrid commented Mar 31, 2020

I mean, I see no reason to support the older

@szejos
Copy link

szejos commented Apr 12, 2020

Can you add a small hint in wiki for those using Visual Studio:
https://docs.microsoft.com/en-us/cpp/build/reference/std-specify-language-standard-version?view=vs-2019

With this flag set to default you get a bunch of errors, since it is not supporting C++ < 17.
Need to change it to C++17.

@Ybalrid
Copy link
Collaborator

Ybalrid commented Apr 12, 2020

@szejos Now that you say that, I thought that it was clearly stated that this library was written in C++17. But it seems that it's not the case... I will add a mention to the README.md file! 😄

As a side note, If you use CMake, you can take inspiration on how we include the library for our CI/Examples. This sets the flags automatically for all compilers that supports it https:/Edhebi/cpp-sdl2/blob/master/examples/CMakeLists.txt#L3

@edhebi
Copy link
Owner Author

edhebi commented Apr 13, 2020

Thanks, @Ybalrid. Note that this will be less of an issue when get a proper cmake configuration.

@edhebi
Copy link
Owner Author

edhebi commented Jun 21, 2020

  • Cmake: build is done. we still need a proper install target and a cpp_sdl2-config.cmake, and it'll be done. I'm still looking for a way to set a list of warnings that doesn't leak to library consumers.

  • package manager: we're now using vcpkg in the CI, wich is an implementation detail for consumers. We don't require any specific way to fetch cmake dependencies as long as we can find_package them.

  • single branch : I actually like it that way.

  • we don't support standards older than c++17

@pyronide
Copy link

I'd like to see this in MSYS2, although I think there would be a problem with cmake, as the current cmake files for SDL2 on MSYS won't link unless SDL_MAIN_HANDLED is defined.

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

No branches or pull requests

4 participants