inexorgame-obsolete / deprecated-cube-engine-inexor

UNMAINTAINED: Please have a look at the vulkan-renderer
https://inexor.org
zlib License
11 stars 1 forks source link

format the entire codebase #417

Open a-teammate opened 7 years ago

a-teammate commented 7 years ago

we have a little problem with inconsistent formatting through the codebase. E.g. spaces are mixed with tabs, sometimes new code accidentally used

void functionname {

instead of

void functionname
{

a clang format file should be created to be used for automatic formatting of the codebase. Sadly, https://github.com/inexor-game/code/wiki/Coding-Standards has very little information as a base for that as yet.

So anyone doing this, would need to make the decision for each bullet point in that config file by reading through some old code (best is to base the decisions mostly on the legacy codebase in e.g. inexor/fpsgame or inexor/engine).

But maybe thats a good way to get in first touch with the code?

terencode commented 7 years ago

But maybe thats a good way to get in first touch with the code?

Agree!

Fohlen commented 7 years ago

https://clang.llvm.org/docs/ClangFormatStyleOptions.html

DonutsInBelly commented 7 years ago

Hi Can I do this?

Fohlen commented 7 years ago

Hello there and welcome to inexor! You don't have to specifically ask to work on a ticket, just pull and rock! @a_teammate and I have agreed that something like https://webkit.org/code-style-guidelines/ probably fits best to the project .. as always maintaining as little as possible is aquired (small modifications to for our code base are ok, but not more)

a-teammate commented 7 years ago

Hey and welcome :) Nah I think it's very good to ask before starting. Otherwise work would be done redundantly a lot

Agreed the webkit file is probably the best starting point. But you'll see :)

DonutsInBelly commented 7 years ago

Thanks for the warm welcome! I'll get started then :smile:

a-teammate commented 6 years ago

Hey there @dominusbelli are you still interested in this, or should we better reassign it? :) Let us know if there are any problems, we are in Riot almost always available!

DonutsInBelly commented 6 years ago

Hi! Sorry I haven't been very active... I got tied up in school and job applications. It might be best to assign it to someone else; I ran into a couple issues in the first week trying to run it and I've also never touched C++. Maybe I'll try a different issue in the future.

bobbybrot commented 6 years ago

No worries @a-teammate @dominusbelli leave this one to me.

bobbybrot commented 6 years ago

Hey @a-teammate, sorry if this is taking a lot longer than it should. I am starting to also get busy from my end as well, but I am slowly working my way through the code base. If it gets to a point where I might be unable to continue work, I will commit and create a pull request on parts of the code base that I have made changes too.

pulkit03 commented 6 years ago

I'd be willing to help if required.

bobbybrot commented 6 years ago

Hey @pulkit03, thanks that would be very helpful. I need to finish reviewing one of the files before I will be confident to hand the task over. Once I am done, I will attach a few screenshots showing you what areas of the code base are done and what hasn't

bobbybrot commented 6 years ago

495, This is the pull request that I have made based on the changes that I have been able to carry out.

bobbybrot commented 6 years ago

The file that I was next to work on is in the client folder (inexor/client/gamemode) called capture_client.h image

bobbybrot commented 6 years ago

If you could start from there and work your way through the codebase, that would be great

pulkit03 commented 6 years ago

@bobbybrot Will get on it once any of the admins approve. :)

a-teammate commented 6 years ago

Sure, @bobbybrot did you use a .clang-format file for the formatting?

This issue might be harder than expected as @mbitsnbites mentioned it broke sth after just formatting everything (you don't need to check that yet. if things change they tend to break. we just need to fix it, we can help you with that, as you currently have problems starting inexor anyways i guess)

the PR seems to be a good beginning, @pulkit03 and @bobbybrot you basically just need to coordinate with each other where you are working currently. If sth strange happens it might help to ask in riot for more help.

bobbybrot commented 6 years ago

Hey @a-teammate, no I did all my adjustments manually and directly.

a-teammate commented 6 years ago

ah okay :) Maybe we could go the next steps by specifying the specific style in a .clang-format file. One would need to find out how to use clangformat with that file, then find a style and iterate until the style fits the old sauerbraten code the best imo :)

mbitsnbites commented 6 years ago

I tried finding a match to the Tesseract code style and got close, but I threw it away since I wanted to modernize the style anyway.

In any case, it's not possible to find a 100% match with clang-format, so you'll have to make some changes to the style.

Also, the excessive use of macros can be problematic. For instance, clang-format does not understand that loopi is a for-loop.

a-teammate commented 6 years ago

I tried around with clang-format myself now and it seems to be embarrassingly static. Not nearly enough options to even control the basic minimum :/ I.e. I think its not asked too much that the formatter can preserve this chunk:

    return v.x <= bo.x+br.x &&
           v.x >= bo.x-br.x &&
           v.y <= bo.y+br.y &&
           v.y >= bo.y-br.y &&
           v.z <= bo.z+br.z &&
           v.z >= bo.z-br.z;

but it doesn't. It just puts everything in one line.

I really expected it to be way more mature.. (i.e. newlines before braces get added differently when they are inside a macro)

Now I ended up fiddeling around with Uncrustify and it gives way better results, although it throws more errors.

If anyone wants to dive into this again (@bobbybrot maybe), i think uncrustify may be easier to tame.

a-teammate commented 6 years ago

Coala is a linter which has additional static analysis qualities, maybe it a good choice for code formatting too. https://coala.io/#/languages, https://docs.google.com/spreadsheets/d/1bm63TQHndmGf3HQ33fp9UEmGKNYI7dTkjMyFIof2PqA/edit#gid=0