Code conventions and standards

Code conventions and standards

Postby freem » 21 Aug 2017, 20:49

Hello.

I am currently hacking around, and noticed that there is no real "howto hack". There is also several points in which code architecture might be improved a lot (yes, it takes lot of time and does not add featires, but I think that it would ease improvements and reduce bugs).

I intend to take some time to do so, but I have several questions to avoid wasting my and your time:

* is cpp11 allowed? cpp14?
* is it ok to remove commented code? It just slows reading the code and pollutes the diffs.
* any coding standard? It seems that there are globally followed rules, but can't really guess them all from source.
* is there is any kind ofAPI or ABI stability to keep? Maybe something network related?
* would it be ok to split manuslly those f*****g long lines that are everywhere? What would be the ideal max width (remember, splitting a screen to see 2 files is damn useful)?
* with a defined codestyle, it could be possible to add a git hook to checkthat they are followed, which would avoid code quality regressions. Would it be ok?
* few things could be imported or exported from/into libs, I am thinking about the (mathematical) vector genric class, for example. What would be the requirements?
* errors are sometimes managed with exceptions (not inheriting std::exception, too) and sometimes with nullchecks. I think it would be good to uniformize that, but to which direction? Anddo you agreewith this?

Finally, I have noticed the point about copyrights of contributions. I am notagainst cc0, but I must admit I prefer cc-by, which is roughly BSD-3 clauses. Itdoes not cost to name contributors, right? ;)

I will probably have several other questions, but let those come in the discussion :)
freem
 
Posts: 17
Joined: 18 May 2015, 19:38

Re: Code conventions and standards

Postby Andrettin » 23 Aug 2017, 15:10

freem {l Wrote}:Hello.

I am currently hacking around, and noticed that there is no real "howto hack". There is also several points in which code architecture might be improved a lot (yes, it takes lot of time and does not add featires, but I think that it would ease improvements and reduce bugs).

I intend to take some time to do so, but I have several questions to avoid wasting my and your time:

* is cpp11 allowed? cpp14?
* is it ok to remove commented code? It just slows reading the code and pollutes the diffs.
* any coding standard? It seems that there are globally followed rules, but can't really guess them all from source.
* is there is any kind ofAPI or ABI stability to keep? Maybe something network related?
* would it be ok to split manuslly those f*****g long lines that are everywhere? What would be the ideal max width (remember, splitting a screen to see 2 files is damn useful)?
* with a defined codestyle, it could be possible to add a git hook to checkthat they are followed, which would avoid code quality regressions. Would it be ok?
* few things could be imported or exported from/into libs, I am thinking about the (mathematical) vector genric class, for example. What would be the requirements?
* errors are sometimes managed with exceptions (not inheriting std::exception, too) and sometimes with nullchecks. I think it would be good to uniformize that, but to which direction? Anddo you agreewith this?

Finally, I have noticed the point about copyrights of contributions. I am notagainst cc0, but I must admit I prefer cc-by, which is roughly BSD-3 clauses. Itdoes not cost to name contributors, right? ;)

I will probably have several other questions, but let those come in the discussion :)


Hey! :)

To answer your questions:

1. C++11 is fine. C++14 could potentially be an issue, as many compilers don't support it yet.
2. I originally commented out (but kept) code that is present in Stratagus itself but not in Wyrmgus for ease of identification, to make integrating new code from Stratagus into Wyrmgus more practical. Now that Stratagus is not being as actively updated, however, there is not as much reason to do so anymore. A very small part of the commented out code are things intended for use in the future (i.e. some of the xBRZ code for the intended zoom mode).
3. I largely kept to the same conventions Stratagus itself used, but those are unfortunately not explicitly laid out anywhere. There are some slight differences in coding standard between the code that I added and the original Stratagus code, since I didn't go back and modify the older code for that purpose. It would indeed be good to lay down the coding standards in written form in the future.
4. IIRC there isn't.
5. Which lines do you mean? For (most) function calls I think a single line looks more organized, but some conditionals indeed have overly long lines.
6. That sounds like a good idea :)
7. What benefits do you see in exporting those things to libraries?
8. Which one do you think is best? AFAIK exceptions are worse for performance than conditionals, so nullchecks should be more efficient?

About contribution copyright, for code contributions either CC0 or the GPLv2 would be alright. CC-BY would unfortunately not be compatible with the engine's license (GPLv2).
Andrettin
Wyrmsun Moderator
 
Posts: 204
Joined: 29 Mar 2015, 19:26

Re: Code conventions and standards

Postby freem » 25 Aug 2017, 20:32

2: it makes merges harder to keep zombie code imo
3: then I will try to determine an astyle config that matches current style.
5: it is more conditionals and "complex" operations than simple calls.
7: in practice, it is more about importing, replacing code from wyrmgus with some of external libs. But putting code into (static) libs can help reducing couple between classes and structures. Benefits are mostly imho easing maintenance effort *if done correctly*. This question is rather theoric, since you seem to be rather alone in the code :)
8: it is not as simple as that. Exceptions makes binaries bigger, cost a lot to throw and in a dynamic library have great chances to break ABI compatibility depending on lot of parameters, but my main argument against them in cpp is that there is no tool to check that all cases are treated. On the other hand, exceptions makes, in theory, safe code easier to write, and have no perf impact while none are thrown. Their advocates also think a code is easier to maintain with them because the error path is distinct.

About exceptions... I would say that in cpp I think it is a bad idea to use them, because there is no way to make the compiler check that all cases where treated. In other languages like java, it is better to use them. But that's a very personnal point of view, and this project is yours, so you should build your opinion.
Anyway, mixing the politics (both have pros and cons) is the worse thing to do, imo.
freem
 
Posts: 17
Joined: 18 May 2015, 19:38

Re: Code conventions and standards

Postby freem » 29 Aug 2017, 20:19

Most of the questions are not really important (I mean, they are quick to change), but I would be very happy if you could reply on the error check point: APIs with or without exceptions are quite different, and since I am hacking around what seems to be a very central piece of the code (layers and map sizes) I would be quite grateful to not do it twice ;)
For now I am putting throw 0 and asserts, but it is... ugly, imho

Oh, and... do you use snake or camel for variables, members, types? I have noticed both and so I doubt...
freem
 
Posts: 17
Joined: 18 May 2015, 19:38

Re: Code conventions and standards

Postby Andrettin » 30 Aug 2017, 18:32

freem {l Wrote}:2: it makes merges harder to keep zombie code imo


Generally I agree, but in this particular case the code wasn't being merged in full; there are divergences between the two code bases which I had to account for. I also found it useful to mark my changes clearly to identify my own mistakes.

3: then I will try to determine an astyle config that matches current style.


Thank you :)

5: it is more conditionals and "complex" operations than simple calls.


I agree, those should be split in multiple lines.

7: in practice, it is more about importing, replacing code from wyrmgus with some of external libs. But putting code into (static) libs can help reducing couple between classes and structures. Benefits are mostly imho easing maintenance effort *if done correctly*. This question is rather theoric, since you seem to be rather alone in the code :)


Replacing things like the Vec2i structs (which are used in a LOT of code) could very well cause bugs to appear, and I'm not sure it would improve maintenance effort all that much.

8: it is not as simple as that. Exceptions makes binaries bigger, cost a lot to throw and in a dynamic library have great chances to break ABI compatibility depending on lot of parameters, but my main argument against them in cpp is that there is no tool to check that all cases are treated. On the other hand, exceptions makes, in theory, safe code easier to write, and have no perf impact while none are thrown. Their advocates also think a code is easier to maintain with them because the error path is distinct.

About exceptions... I would say that in cpp I think it is a bad idea to use them, because there is no way to make the compiler check that all cases where treated. In other languages like java, it is better to use them. But that's a very personnal point of view, and this project is yours, so you should build your opinion.
Anyway, mixing the politics (both have pros and cons) is the worse thing to do, imo.


freem {l Wrote}:Most of the questions are not really important (I mean, they are quick to change), but I would be very happy if you could reply on the error check point: APIs with or without exceptions are quite different, and since I am hacking around what seems to be a very central piece of the code (layers and map sizes) I would be quite grateful to not do it twice ;)
For now I am putting throw 0 and asserts, but it is... ugly, imho


I had understood you thought adding exceptions wouldn't be worth it, since the engine is programmed in C++?

Oh, and... do you use snake or camel for variables, members, types? I have noticed both and so I doubt...


Good question; I use snake_case for variables that are defined and used within functions, and CamelCase for variables used in class/struct definitions. I haven't edited the older code to conform to that, though.
Andrettin
Wyrmsun Moderator
 
Posts: 204
Joined: 29 Mar 2015, 19:26

Re: Code conventions and standards

Postby freem » 31 Aug 2017, 00:22

About vec2t, I do not agree, but see and understand the point, and you're the boss here :)

For exceptions, yes, I tend to not like them, for various reasons, that I tried to expose by staying as neutral as possible. Now, it is *your* project and I try to not impose my opinions, especially when they are not the most usual ones. People tend to use exceptions in practice, and coding a safe code without exceptions is not classic. Note that in current state, i have seen no exception management and only rare return checks.
Basically, the classic way to go without exceptions is the C way, with factory or init methods, with checked returns. On a personal project, I have enhanced this with a special template for returns that "encourages" the value to be checked (because, basically, it is a pair with one type being a bool set to true, and the only way to acces data is by moving it to it's destination, which assert(flag). of course, there is a way to check the flag without taking value, and i intend to maybe add a custom way to signal errors. It is not perfect, of course, but on debug mode you know exactly where the code fails if the situation was not checked, which is difficult to forgot to do due to the api of the template. well, just my implementation of a strange idea).
The exception way, imply imho the risk of silent crashes that you can never really know where they come from except if you do an explicit try...catch... at each eventually throwing call. But, this is the classic cpp way and stl do throw. Also, it allows to have only one test for a bloc of code where several calls can throw.
So, why do I add ugly throw 0? Because it is easy to grep that kind of ugly stuff (otherwise i would throw more pertinent exceptions with real error messages) waiting for the project's boss (you) opinion :)

And for internal vars, ok, if at a moment I need to change a name or add one, I guess I should use a snake one?


PS: sorry for the walls of text
freem
 
Posts: 17
Joined: 18 May 2015, 19:38

Re: Code conventions and standards

Postby Andrettin » 31 Aug 2017, 17:57

freem {l Wrote}:For exceptions, yes, I tend to not like them, for various reasons, that I tried to expose by staying as neutral as possible. Now, it is *your* project and I try to not impose my opinions, especially when they are not the most usual ones. People tend to use exceptions in practice, and coding a safe code without exceptions is not classic. Note that in current state, i have seen no exception management and only rare return checks.
Basically, the classic way to go without exceptions is the C way, with factory or init methods, with checked returns. On a personal project, I have enhanced this with a special template for returns that "encourages" the value to be checked (because, basically, it is a pair with one type being a bool set to true, and the only way to acces data is by moving it to it's destination, which assert(flag). of course, there is a way to check the flag without taking value, and i intend to maybe add a custom way to signal errors. It is not perfect, of course, but on debug mode you know exactly where the code fails if the situation was not checked, which is difficult to forgot to do due to the api of the template. well, just my implementation of a strange idea).
The exception way, imply imho the risk of silent crashes that you can never really know where they come from except if you do an explicit try...catch... at each eventually throwing call. But, this is the classic cpp way and stl do throw. Also, it allows to have only one test for a bloc of code where several calls can throw.
So, why do I add ugly throw 0? Because it is easy to grep that kind of ugly stuff (otherwise i would throw more pertinent exceptions with real error messages) waiting for the project's boss (you) opinion :)


Thanks for the explanation! It sounds good to me.

And for internal vars, ok, if at a moment I need to change a name or add one, I guess I should use a snake one?


Yup :)
Andrettin
Wyrmsun Moderator
 
Posts: 204
Joined: 29 Mar 2015, 19:26

Re: Code conventions and standards

Postby freem » 01 Sep 2017, 17:00

No problem for the explanations. I am doing decent progress on the map's dimensions refacto (oh, sorry, I forgot to mention that it's me the guy who annoy you with patches) and so I wonder what you will prefer for the error checks way.
freem
 
Posts: 17
Joined: 18 May 2015, 19:38

Re: Code conventions and standards

Postby freem » 10 Sep 2017, 16:10

I took the opportunity to have a decent computer and unproxified connection to send a PR related to the refactoring.
As I said in the related comment, there are lot of lines changed, many small things, and I think there might be typo in the code (it compiles, but I had not enough time to really try... sadly).
Take time to review the changes, and good luck.

Now, some thoughts about my next steps (it's just to keep track of my thoughts, but comments are welcome).
I have notived several patterns that could be refactored (and several other variables that could be merged in vec2t):

* functions width and heights parameters from the same data set
* width*height operations are frequent enough to be factorised in inline function
* there are lot of near identical calls to complex min/max functions
* a way to build to foreach-like call on domensions would improve both readability and performances (perfs because in C and C++, arrays are row-based, so iterating on x and then y means less efficient use of cpu caches)

It's all I can think for now, luckily changing those things should be doable by reasonnable patches (unlike the huge mess I sent in the pr).
freem
 
Posts: 17
Joined: 18 May 2015, 19:38

Who is online

Users browsing this forum: No registered users and 1 guest