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

[docs] Update style guidelines #14665

Merged
merged 1 commit into from Feb 18, 2019

Conversation

@pkerling
Copy link
Member

commented Oct 21, 2018

Description

Update code style guidelines: Make it more consistent (description and examples), group some similar things together, and add some more rules/hints.

Motivation and Context

During reviews often things come up that we do not have any position on.

Other

We may also want to have a short guide for Git commit messages; should that be part of this document or rather be separate?

I just tried to type something up to have a basis for discussion. Constructive criticism and feedback welcome :-)

@pkerling pkerling self-assigned this Oct 21, 2018

@pkerling pkerling force-pushed the pkerling:code-style-guidelines branch from 5d43178 to 0be9bb1 Oct 21, 2018

@pkerling pkerling requested review from Paxxi, lrusak, garbear, Rechi and ace20022 Oct 21, 2018

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2018

I pinged some people I know I've talked to about this or might be interested, but the discussion is not meant to be limited to those in any way.

@Paxxi

Paxxi approved these changes Oct 21, 2018

Copy link
Member

left a comment

Some C++14 is used in windows only code as msvc haven't adhered to strict versioning flags. The one that comes to mind is make_unique. Not sure if it's worth mentioning as it might be confusing.

Nice work 😀👍

Show resolved Hide resolved docs/CODE_GUIDELINES.md Outdated
Show resolved Hide resolved docs/CODE_GUIDELINES.md Outdated

❌ Bad:
```cpp
void test(std::string const& a);

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

This brings up two points - we want to avoid const on primitive parameters, e.g. test(const int a), and also we allow const after the modifier when we actually want const values, e.g.

class Foo
{
  const Bar* const m_immutable;
};

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

This brings up two points - we want to avoid const on primitive parameters, e.g. test(const int a)

Don't have a strong opinion on this one - I think it's fine to do so if you want to emphasize that the function should not change the variable in its body (even if it would not be visible to the caller anyway).

and also we allow const after the modifier when we actually want const values

Yeah that's a different type of const but maybe I can improve the wording.

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

Don't have a strong opinion on this one - I think it's fine to do so if you want to emphasize that the function should not change the variable in its body (even if it would not be visible to the caller anyway).

I am against this, because I don't think this internal detail should be exposed to external callers. However, inside a function, I encourage the use of const variables as much as possible -- see how rust defaults everything to const and requires mutable to be declared explicitly.


```cpp
#include "PVRManager.h"

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

Most of our code doesn't include this newline

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

Well, probably 99 % of the code won't follow the guidelines in their updated form :-D

So the question is: Going forward, do we want to have it?

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

I think the newline adds no benefit, it's obvious throughout the code that the first header is distinctly the source's own.

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

I think it's mainly to separate the groups that have alphabetical ordering within them

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

What are others' opinions?

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Oct 23, 2018

Member

Having a newline would make it a consistent separator of sorted groups, but not a strong opinion

* Own header file
* Other Kodi includes
* C and C++ system files
* Other libraries' header files

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

We generally put other libraries' headers before system files

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

same as above: What do we want to have?

Personally I'd put the system headers before other libraries for hierarchical reasons, but I can live with either order.

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

The way I see it, we go from the most our-code, to the least-our-code. Kodi includes are ours, dependencies are kinda ours, and system headers are least ours.

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Oct 23, 2018

Member

Can we clarify if sort order is case sensitive or not. The example implies a...z,A...Z, but good to state it explicitly if that is what we want. I don't mind which and I have seen both in existing code (or maybe it was just a jumble).

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 23, 2018

Author Member

Can we clarify if sort order is case sensitive or not. The example implies a...z,A...Z,

I don't think this is implied by the example. It explicitly says: "Place directories before files", which is I think the rule that is followed. Do we actually have cases where case matters (pun not intended) after putting dirs first?

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Oct 24, 2018

Member

Doh, I have clearly never understood what "Place directories before files" meant! But that does make sense of what I have seen, sorry for noise

Show resolved Hide resolved docs/CODE_GUIDELINES.md Outdated
Show resolved Hide resolved docs/CODE_GUIDELINES.md Outdated
#### Global Variables
Prefix global variables with *g_*
#### Global variables
Prefix global variables with `g_`

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

No new globals, period.

This comment has been minimized.

Copy link
@pkerling

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

That's not too offensive, because std::unique_ptr construction and destruction is trivial. However, I'm greatly concerned that g_WlMessagePump isn't reset() deterministically, because thread destructors are extremely non-trivial.

I stand by my no-global requirement, because they can always be encapsulated as a static variable in a class with no downside.

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

I stand by my no-global requirement, because they can always be encapsulated as a static variable in a class with no downside.

What's the upside?

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

Encapsulation. It's clear who the variable belongs to. A class-local static is more specific than a file-local static, because a file can have multiple classes.

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

Also, the no-globals rule forces the developer to consider a better place for the object, and in the process they might find a way to avoid a global at all. Like above, the result is less important than engaging the developer in a way to consider architectural improvements. Adding a global is more lazy than adding a static class variable.

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

Here's another point, when the global is owned by a class, it's very clear who is in charge of creating/destroying the global. A global without a class owner omits responsibility for its lifecycle management. In your cited example, it is less clear that CWinEventsWayland is responsible for creating and destroying the object (and indeed, fails to destroy it deterministically).

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 24, 2018

Author Member

Encapsulation. It's clear who the variable belongs to. A class-local static is more specific than a file-local static, because a file can have multiple classes.

In your cited example, it is less clear that CWinEventsWayland is responsible for creating and destroying the object (and indeed, fails to destroy it deterministically).

Still a bit torn about the example, I could make the unique_ptr a member of CWinEventsWayland, but only if I at least forward-declare CWinEventsWaylandThread. I wanted to avoid that as it is an implementation detail of CWinEventsWayland and does not need to be public in any form. That's why it's a file-local utility class.

Like above, the result is less important than engaging the developer in a way to consider architectural improvements.

Agreed, although I think that the proposed wording already makes it pretty clear that people have to consider alternatives.

*
* Call only from main thread
*/
void SetState(CSizeInt size, int scale, IShellSurface::StateBitset state);

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

Maybe include an example where the parameters and return type are doxy'd?

EDIT: I saw below, I think that parameters should be explicitly documented, even if it's trivial (though trivial parameter descriptions are discouraged, of course)

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

Maybe include an example where the parameters and return type are doxy'd?

Good idea. Do you have one in mind?

I saw below, I think that parameters should be explicitly documented, even if it's trivial (though trivial parameter descriptions are discouraged, of course)

Not completely sure, but I tend to disagree. Comments should only be there if they add value. What value do you think is added by having a doxy comment like in the (intentionally bad) example?

This comment has been minimized.

Copy link
@Paxxi

Paxxi Oct 21, 2018

Member

Messaging/helpers should have parameters and return types documented.
Appmessenger and surrounding code is well documented (quality is debatable 😀)

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

In this case, my argument isn't for the result but the habit - developers should practice documenting everything. Oftentimes, I'll document three parameters, the first two will be trivial but the third I expand upon. The "noise" from the first two is worth the reminder to myself to expand upon the more opaque parameter.

It's like including the many headers in our PR templates - the dev is free to ignore and delete any headers, but I want them to be conscious that they're doing so. It should take extra effort to document less, not the other way around.

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

Messaging/helpers should have parameters and return types documented.

Tried a utils one.

Related question: Do we want to have the * on the left side of the doxy comments? I kind of like them because it makes it more visually distinct in the editor, but I'm open for opinions.

In this case, my argument isn't for the result but the habit - developers should practice documenting everything.

I'm seeing the opposite problem - if you get into the habit of "documenting" everything by adding boilerplate text that does not add information, you not only clutter the code, but may also grow into an automatism that prevents you from writing actually meaningful comments.

Prefer the use of `nullptr` instead of `NULL`. `nullptr` is a typesafe version and as such can't be implicitly converted to `int` or anything else.

### 12.3. auto
### 10.5. `auto`

Feel free to use `auto` wherever it improves readability. Good places are iterators or when dealing with containers.

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

We should discourage auto when it isn't necessary for readability, as it is opaque compared to using the actual types.

This comment has been minimized.

Copy link
@Paxxi

Paxxi Oct 21, 2018

Member

I prefer using auto whenever the type is obvious from context

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 21, 2018

Author Member

With @garbear on this one

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-auto

Exception: Avoid auto for initializer lists and in cases where you know exactly which type you want and where an initializer might require conversion.

https://google.github.io/styleguide/cppguide.html#auto

Use auto to avoid type names that are noisy, obvious, or unimportant - cases where the type doesn't aid in clarity for the reader. Continue to use manifest type declarations when it helps readability.

But maybe that's actually also what you mean with "obivous from context" @Paxxi ?

This comment has been minimized.

Copy link
@Paxxi

Paxxi Oct 21, 2018

Member

Yeah, if a method is named GetFileItems I don't need to spell out that it's a vector<CFileItem>

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

What's obvious to you may not be obvious to a new dev unfamiliar with the code.

This comment has been minimized.

Copy link
@Paxxi

Paxxi Oct 21, 2018

Member

If you know size_t is 64bit on windows then auto isn't an issue and if you don't know it auto won't make a difference

This comment has been minimized.

Copy link
@garbear

garbear Oct 21, 2018

Member

It isn't about knowing that size_t is 64bit, it's about knowing that vec.size() returns a size_t. Obviously most devs know this, but being explicit is explanatory to new devs.

This comment has been minimized.

Copy link
@a1rwulf

a1rwulf Oct 22, 2018

Member

Yeah, if a method is named GetFileItems I don't need to spell out that it's a vector<CFileItem>

You can only assume so if you were the author, it could also be a list<CFileItem> or even a map.
And yes, I know the rule to prefer vector over list when we talk about a small amount of entries, but that's beside the point here.
Just saying that GetFileItems is a valid function name that can return basically any container.

I'm more with @garbear here, I hate over usage of auto.
I mostly use it in range-based for loops and not so much anywhere else.

This comment has been minimized.

Copy link
@Paxxi

Paxxi Oct 23, 2018

Member

That is true. I don't believe it to be an issue in practice though. And when in doubt your ide will display the type on hover or similar method.
I'm not sure it makes sense to force a style optimized for simple text editor users.
Either don't mention auto or say something like, use your judgment and when in doubt write the type

This comment has been minimized.

Copy link
@DaveTBlake

DaveTBlake Oct 23, 2018

Member

Also with garbear on readability (I read with my eyes, not pointing the mouse)

Show resolved Hide resolved docs/CODE_GUIDELINES.md Outdated
@garbear

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

Nice. I left all the comments I had.

@pkerling pkerling requested a review from a1rwulf Oct 21, 2018

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2018

Some C++14 is used in windows only code as msvc haven't adhered to strict versioning flags. The one that comes to mind is make_unique. Not sure if it's worth mentioning as it might be confusing.

I wouldn't mention it at the moment, it would just confuse people. I kind of hope that we'll be switching to C++14 for v19 anyway 🙏

@Paxxi

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

Okay. You appear to have strong feelings on this so I accept that. Go ahead with @garbear original comments.

@notspiff

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

@lrusak

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

@pkerling I don't have any strong feeling either way. I'd rather let the experts determine this stuff and I'll follow with whatever is decided (as long as it's for good reason) 👍

Also, here is the rich text for those who want to see what it actually looks like 💯
https://github.com/pkerling/xbmc/blob/code-style-guidelines/docs/CODE_GUIDELINES.md


### 9.2. Doxygen

New classes and functions are expected to have Doxygen comments describing their purpose, parameters, and behavior in the header file. However, do not describe trivialities - it only adds visual noise. Use the Qt style with exclamation mark (`/*! */`) and backslash for doxygen commands (e.g. `\brief`).

This comment has been minimized.

Copy link
@garbear

garbear Oct 23, 2018

Member

Instead of:

However, do not describe trivialities - it only adds visual noise.

How about:

Avoid trivialities - this is a sign that you should be more descriptive.

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 23, 2018

Author Member

Hmm I'm unsure about that. It could just encourage people to write trivialities more verbosely :-)

In some cases there is just nothing useful to say.

This comment has been minimized.

Copy link
@garbear

garbear Oct 23, 2018

Member

Sure, your wording is fine.

This comment has been minimized.

Copy link
@pkerling

pkerling Oct 23, 2018

Author Member

I get what you want to say though. If you find a wording that combines both, let me know...

/*!
* \brief Frobnicate a parameter
* \param param parameter to frobnicate
* \result frobnication result

This comment has been minimized.

Copy link
@garbear

garbear Oct 23, 2018

Member

lol

@xbmc xbmc deleted a comment from a1rwulf Oct 23, 2018

@xbmc xbmc deleted a comment from a1rwulf Oct 23, 2018

@pkerling pkerling referenced this pull request Oct 25, 2018

Merged

Code quality improvements #14672

1 of 7 tasks complete
@pkerling

This comment has been minimized.

Copy link
Member Author

commented Oct 25, 2018

#14672 has seen some discussion on initialization with {} that I'd like to respond to and I feel would be better suited for this PR.

And I don't like the init of primitive types with () or {}. Especially if no truncation can occur which wasn't intended.

Initialization with () is off the table anyway. The other is for consistency. You shouldn't have to think about whether you could truncate something or not, you should just get an error if you do. And you should explicitly static_cast for truncating so that it is clear it is what you want. That's my opinion at least.

Value/zero-initialization via braces for primitive types looks weird IMO.

As @garbear already mentioned, this is purely a matter of habit and I don't think we should decide based on that. I did try not to push this too hard in the style guidelines though and only recommend using {}. C++ core guidelines also acknowledge this: "Old habits die hard, so this rule is hard to apply consistently, especially as there are so many cases where = is innocent."

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-list

When skimming through code like that, you have to also look at the type (worse when it's a typedef to a primitive) and you have to know that primitive types are value-initialized to 0.

The latter part does not seem very relevant, it's also mostly a matter of being in the habit. You'll know about it after you've seen it 2, 3 times.

Do note that I am not arguing against writing int a{0} (or similar) at all. It's all about intent. If you feel it makes the code more readable when the 0 is there - by all means do put it there. If it's clear anyway and would be superfluous - leave it out. Maybe this is not clear from the description in the style guide?

Granted, every sane person would assume that, but it's extra mental effort for no actual reason besides using more modern syntax and 2 less characters. C++ has a bajillion ways to initialize shit, don't make it more complicated

Reiterating: {[...]} is applicable pretty much anywhere, so it is the one initialization solution that has the potential to make it less complicated - if applied consistently.

I do know that this will be hard to impossible to achieve completely with the Kodi code base (like most of these rules really). So I'd in principle be fine with saying we recommend the int a = 1 syntax (and ignore narrowing and other problems) for primitive types because {} not very popular yet even though it's technically superior.

@DaveTBlake

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

So I'd in principle be fine with saying we recommend the int a = 1 syntax (and ignore narrowing and other problems) for primitive types because {} not very popular yet even though it's technically superior.

I would prefer that - after nearly 40 year of reading code in many different languages, a simple = is so easy to read and understand in variable initialisation.

@garbear

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

I second @DaveTBlake - a simple = is easier to read and understand. And that's what's most important. Maybe in a few years {} will take over, but now I think our rule should be {} is for non-primitive types, and = for primitive types is allowed because it improves readability.

@Paxxi

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

I don't think we should be too strict here. Goal IMO should be to get contributors to do the right thing and not face nitpicking over style when submitting a pr.
Allowing common styles seems fine to me unless they cause serious readability issues.
If say we want to force init with {} in the future we should run clang-tidy on everything so new contributors only see the approved style and get it right by following surrounding code style.

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2018

OK, so the direction seems to be to prefer = for zero and explicit initialization of primitive types, but prefer {} for non-primitive types (so you'd write e.g. CPointInt p1{3, 5} and not CPointInt p1(3, 5)). Did I get that right? I'm not sure the last point would be very helpful, since the upside of {} is mostly that you can use it for everything - so limiting it to one specific case kind of defeats its purpose. It still gets you around most vexing parse, though.

And do we want to make that a fixed rule for new code or allow both alternatives by only giving a recommendation?

@Paxxi

This comment has been minimized.

Copy link
Member

commented Oct 29, 2018

I would make it a recommendation. Both have upsides and drawbacks so allowing both is most pragmatic.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

Can we merge whatever we have now?

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

@MartijnKaijser I'd go over it once more and merge for v19. That ok?

Haven't forgotten about it, just busy atm and this isn't high priority.

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

I would make it a recommendation.

Done - ready to merge from my side. Everybody happy? :-)

@pkerling pkerling removed the WIP label Feb 10, 2019

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

squash? :)

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2019

After I got some +1s and am sure I don't have to edit something again, yes ;-D

@a1rwulf
Copy link
Member

left a comment

I'm fine with it.

@pkerling pkerling force-pushed the pkerling:code-style-guidelines branch from 17e54a4 to eceae78 Feb 18, 2019

@pkerling

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

Thanks all for the fruitful discussion!

@pkerling pkerling merged commit 5e82ff9 into xbmc:master Feb 18, 2019

@pkerling pkerling deleted the pkerling:code-style-guidelines branch Feb 18, 2019

@Rechi Rechi added the v18 Leia label Feb 26, 2019

@Rechi Rechi added this to the Leia 18.2-rc1 milestone Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.