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

Standardize implementation guidelines in the wiki: get methods. #318

Closed
IAmNotHanni opened this issue Jan 14, 2021 · 4 comments · Fixed by #311
Closed

Standardize implementation guidelines in the wiki: get methods. #318

IAmNotHanni opened this issue Jan 14, 2021 · 4 comments · Fixed by #311
Labels
cat:documentation documentation

Comments

@IAmNotHanni
Copy link
Member

IAmNotHanni commented Jan 14, 2021

We already talked about how to implement get methods consistently:

Rules

  • Name: Don't use prefix get_. Give the get method the same name as the resource it returns.
  • For complex types (std::vector, std::string), return a const reference.
  • Don't const the return type for simple types (int, float), because this prevents move semantics to be applied.
  • For simple types (int, float), just copy the return value.
  • Mark get methods as [[nodiscard]] in the header file only.
  • Mark get methods as const, so they don't change members.
  • Do not add documentation for get methods, since it is self-explanatory.
  • Keep get methods directly in the header file.
  • Do not add inline since get methods in header files are always inlined.
  • The get method should not run any other code, like checking if the value is actually valid. Since we are using RAII, the value to return must be in a valid state anyways.
  • Use operator overloading sparingly.

Examples

[[nodiscard]] const glm::vec3& position() const {
    return m_position;
}
[[nodiscard]] float aspect_ratio() const {
    return m_aspect_ratio;
}
@IAmNotHanni IAmNotHanni added the cat:documentation documentation label Jan 14, 2021
@IceflowRE
Copy link
Member

  • Add a note in the contributing guide to read the development section carefully.

@IAmNotHanni
Copy link
Member Author

#222

@IceflowRE
Copy link
Member

@IAmNotHanni
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:documentation documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants