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

Remove unnecessary typedef's #181

Closed
4 of 15 tasks
ischoegl opened this issue Aug 2, 2023 · 2 comments · Fixed by Cantera/cantera#1568
Closed
4 of 15 tasks

Remove unnecessary typedef's #181

ischoegl opened this issue Aug 2, 2023 · 2 comments · Fixed by Cantera/cantera#1568
Assignees
Labels
work-in-progress An enhancement that someone is currently working on

Comments

@ischoegl
Copy link
Member

ischoegl commented Aug 2, 2023

Abstract

A major version step from 2.6 to 3.0 could be used to remove some legacy typedef's where using standard types would clarify what those types are (after introducing using std::vector;, savings of length are mostly marginal)

There are two different 'compositions', which are identical (one typedef is worth keeping in this instance):

It would also be possible to eliminate std:: in many instances (due to newly introduced using std::xyz:

Motivation

Improve readability of code (and Doxygen documentation):

  • using standard types where applicable ensures that new users understand what types are being used
  • new features oftentimes leverage existing code, where copy/paste of outdated syntax perpetuates issues, leading to unnecessary review feedback (if caught)
  • shorten function signatures (if std:: is omitted where possible)

Possible Solutions

Simple find/replace in *.h/*.cpp files.

@ischoegl ischoegl added the feature-request New feature request label Aug 2, 2023
@speth
Copy link
Member

speth commented Aug 2, 2023

I'd say that making these changes has very little to do with the 3.0 version change, assuming we leave the old typedefs available but marked as deprecated. However, this might be as good an opportunity as we're going to get in terms of having relatively few major PRs open (or large pre-PR projects that I'm aware of) that will require significant manual conflict resolution to rebase them.

One somewhat related change that had been on my mind, in that it would touch a lot of the same lines of code, is making full use of the C++11 override specifier, and removing the virtual keyword from overrides (i.e., virtual should only appear on the base class declaration of a method). Currently, we only do this for a handful of classes that are either recent additions or have had major recent changes.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 2, 2023

Ok. From my perspective, these are changes that are easily implemented but somewhat of a pain to review, which is the reason I was asking. I'm 👍 with moving forward, although I'd like to resolve the virtual in a different enhancement/PR (which I'd be happy to review). If you're ok as well, I'll wait until the current slew of PR's is merged, and start ticking the items off thereafter.

@ischoegl ischoegl added work-in-progress An enhancement that someone is currently working on and removed feature-request New feature request labels Aug 2, 2023
@ischoegl ischoegl self-assigned this Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work-in-progress An enhancement that someone is currently working on
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants