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

Add a C++ usage guidelines page #3668

Merged
merged 2 commits into from
Jul 12, 2021

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 9, 2020

This page describes C++ features that are used and allowed in Godot's codebase. It's based on community consensus over the last months, as Godot recently moved to C++17 in the master branch.

I've probably missed many things, so please leave comments. That said, the C++ style we expect is now (mostly) set in stone, so most disallowed features written here are not negotiable (such as auto). Please keep the discussion on-topic 🙂

I know @hpvb intended to write this page at first, but I decided to do it to help speed up the process.

This closes godotengine/godot#9694.

Lambdas
^^^^^^^

The use of lambdas is generally not allowed, unless they can greatly help with
Copy link
Member Author

@Calinou Calinou Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote the part about lambdas this way because we have a few instances of lambdas in the current editor code. These could be rewritten into macros, but it's not really convenient.

We also have a handful of auto keywords in use, but that can be changed more easily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say:

Lambdas should be used conservatively when they make code effectively faster or simpler, and do not impede readability.

@kyleguarco
Copy link

Can we add a short section on the appropriate usage of switch cases?

I'm bringing it up because of its inconsistent use throughout the code base, specifically on _notification functions. Some use switches, others use really long if / if-else blocks.

Examples:

@Calinou
Copy link
Member Author

Calinou commented Jul 30, 2020

@FlashDaggerX Thinking about it, I think we should always use switch in _notification functions, even if there are only 1 or 2 cases. This is to make refactoring easier. When you add new cases, you don't want to spend time converting the if block to a switch block.

@Ansraer
Copy link

Ansraer commented Sep 12, 2020

Could someone explain the reasoning behind disallowing for-range-loops to me?
The docs only mention that they are not explicit enough, but I don't really feel like that's the case. As long as we are only iterating over not overly complex data it should be quite obvious what is happening.

Plus I would argue that the readability and ease of use make them beneficial for "beginner contributors (who may not be
professional C++ programmers)". In my experience, it is a LOT easier to understand for(CustomObject obj : list) than a 70+ characters long custom loop statement.

@NathanLovato NathanLovato added the content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features label Oct 10, 2020
@NathanLovato NathanLovato added this to the Godot 4.0 milestone Oct 10, 2020
@NathanLovato
Copy link
Contributor

What's the PR's status? What's missing? Is there a way we can review and merge what's available and consider extending it later?

@Calinou
Copy link
Member Author

Calinou commented Oct 28, 2020

@NathanLovato I think this is good to merge as-is, but it would be good to get @akien-mga's approval (especially the part about lambdas).

@Calinou Calinou force-pushed the add-cpp-usage-guidelines branch 2 times, most recently from 7a7b899 to b9dfa42 Compare December 2, 2020 14:37
about/faq.rst Outdated Show resolved Hide resolved
@Calinou Calinou force-pushed the add-cpp-usage-guidelines branch 2 times, most recently from 9c72241 to 9c4954c Compare January 29, 2021 21:01
@fire fire self-requested a review March 4, 2021 17:27
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a read, and the significant points about autos, STL, and lambdas have been commonly agreed upon by the contributor team.

This page describes C++ features that are used and allowed in
Godot's codebase. It's based on community consensus over the last
months, as Godot recently moved to C++17 in the `master` branch.
@mhilbrunner mhilbrunner merged commit 0be0e91 into godotengine:master Jul 12, 2021
@Calinou Calinou deleted the add-cpp-usage-guidelines branch August 3, 2021 16:05
@Calinou
Copy link
Member Author

Calinou commented Aug 19, 2021

Should this page be pushed to 3.4 as-is, with a note saying that it only applies to the master branch? This would help people find it more easily.

We could maintain a dedicated page for 3.x, but it might give people a false impression of what's actually allowed in Godot's C++ codebase since 3.x is restricted to C++14.

@YuriSizov
Copy link
Contributor

I think that notable differences and general code compatibility warning should be a part of the page anyway. People still support 3.x actively, so we should mention the caveats in the docs. Then it wouldn't matter in which version we store the page, as it would be applicable to the development in general.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content:new page Issues and PRs related to creation of new documentation pages for new or undocumented features enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Status of C++ in Godot