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

Rename typedefs #17

Merged
merged 17 commits into from
Jun 1, 2021
Merged

Rename typedefs #17

merged 17 commits into from
Jun 1, 2021

Conversation

brownj85
Copy link
Collaborator

@brownj85 brownj85 commented May 27, 2021

Context:
We have decided to use camel cases for type aliases in accordance with accordance with Google style. We have also decided against the use of _t suffixes for aliases of non-fundamental types. The use of _t suffixes is reserved for aliases of fundamental types (e.g NodeID_t).

Description of the Change:
Changed all type aliases to camel case. Removed _t suffix from all aliases of non-fundamental types.
Also removed type aliases from test files when they were only used once or twice.

Benefits:
Code quality, consistent style.

Possible Drawbacks:

Related GitHub Issues:

@brownj85 brownj85 requested review from Mandrenkov and mlxd May 27, 2021 18:58
@github-actions
Copy link

github-actions bot commented May 27, 2021

Test Report (C++) on Ubuntu

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
240 tests ±0  240 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
520 runs  ±0  520 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit e1b1774. ± Comparison against base commit e1b1774.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 27, 2021

Test Report (C++) on MacOS

    1 files  ±0      1 suites  ±0   0s ⏱️ ±0s
240 tests ±0  240 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
520 runs  ±0  520 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit e1b1774. ± Comparison against base commit e1b1774.

♻️ This comment has been updated with latest results.

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

While overall I think this is a good idea to leave _t to fundamental types, I do worry that many of the new type names can be mistaken for variable names (easy fix and easily spotted, I know).

I think that following https://google.github.io/styleguide/cppguide.html#Type_Names would be better for clarity, where all type names start with a capital letter to differentiate them from potential variable names.

@Mandrenkov Mandrenkov added the code quality 🎓 Improvements to code quality label May 28, 2021
Copy link
Collaborator

@Mandrenkov Mandrenkov left a comment

Choose a reason for hiding this comment

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

Well done, Jack! Looks great!

I will leave the final decision up to you, but I would either follow Lee's suggestion and use CamelCase type names or take a page out of the standard library and use a _type suffix (e.g., see the member types of std::vector or std::pair). Just removing the _t suffix works but may be prone to shadowing.

.github/CHANGELOG.md Outdated Show resolved Hide resolved
@trevor-vincent
Copy link
Contributor

trevor-vincent commented May 31, 2021

Jack, should this be changed in the README.md example (and any relevant examples in docs) as well?

@brownj85
Copy link
Collaborator Author

Well done, Jack! Looks great!

I will leave the final decision up to you, but I would either follow Lee's suggestion and use CamelCase type names or take a page out of the standard library and use a _type suffix (e.g., see the member types of std::vector or std::pair). Just removing the _t suffix works but may be prone to shadowing.

I went with Lee's suggestion in the end, I think it makes sense since Google style is closest to what we've been using thus far.

@brownj85 brownj85 requested a review from Mandrenkov May 31, 2021 19:00
Copy link
Collaborator

@Mandrenkov Mandrenkov left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Looks great! Nothing to add from my side.

@brownj85 brownj85 merged commit e1b1774 into main Jun 1, 2021
@brownj85 brownj85 deleted the rename-typedefs branch June 1, 2021 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality 🎓 Improvements to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants