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.

Sign up for GitHub

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

Jump to bottom

refactor!: replace one_rep with reference<D, U> #261

Merged
merged 15 commits into from
Mar 18, 2021
Merged

refactor!: replace one_rep with reference<D, U> #261

merged 15 commits into from
Mar 18, 2021

Conversation

JohelEGP
Copy link
Collaborator

@JohelEGP JohelEGP commented Mar 17, 2021 •
edited
Loading

Resolves #243.

Drive-by:

  • Add rad unit constant.
  • Use pragmas for -Wshadow and -Wmissing-braces.

mpusz
mpusz requested changes Mar 18, 2021
View reviewed changes
Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

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

Great work!!! Thanks!

Please see my comments...

src/core/include/units/quantity.h Outdated Show resolved Hide resolved
src/core/include/units/quantity.h Outdated Show resolved Hide resolved
src/core/include/units/quantity.h Outdated Show resolved Hide resolved
src/core/include/units/reference.h Outdated Show resolved Hide resolved
src/core/include/units/reference.h Outdated Show resolved Hide resolved
src/core/include/units/reference.h Outdated Show resolved Hide resolved
@JohelEGP
Copy link
Collaborator Author

Note to self: Update docs.

@JohelEGP
Copy link
Collaborator Author

While it looks great in code, I'm not sure about updating the documentation from "unit constant(s)" to "reference(s)". I'll keep those as "unit reference(s)".

@mpusz
Copy link
Owner

mpusz commented Mar 18, 2021

"quantity references"?

mpusz
mpusz requested changes Mar 18, 2021
View reviewed changes
src/core/include/units/quantity.h Outdated Show resolved Hide resolved
src/core/include/units/quantity.h Outdated Show resolved Hide resolved
src/core/include/units/reference.h Outdated Show resolved Hide resolved
test/unit_test/static/unit_constants.cpp Show resolved Hide resolved
mpusz
mpusz requested changes Mar 18, 2021
View reviewed changes
src/core/include/units/reference.h Outdated Show resolved Hide resolved
@mpusz
Copy link
Owner

mpusz commented Mar 18, 2021

BTW, while adding Hidden Friends please always add:

  // Hidden Friends
  // Below friend functions are to be found via argument-dependent lookup only

See more in http://wg21.link/p1601.

@JohelEGP
Copy link
Collaborator Author

There's https://wg21.link/hidden.friends now. Should I still add that?

@mpusz
Copy link
Owner

mpusz commented Mar 18, 2021

Yes, I believe we should add those as later on it may be needed to provide appropriate Remarks: ... clause while writing standardeese. Those comments are meant to remind us about this.

@JohelEGP
Copy link
Collaborator Author

The standard iterator and range adaptors have no such wording. [hidden.friends] makes that redundant.

@mpusz
Copy link
Owner

mpusz commented Mar 18, 2021

I believe that this paper was provided only recently and it is a recommendation for the upcoming proposals. Anyway, I hope we will be able to check this in practice in a few years from now ;-)

@JohelEGP
Copy link
Collaborator Author

The docs: commit is for the header files. The .rst under docs/ will come later.

mpusz
mpusz requested changes Mar 18, 2021
View reviewed changes
Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

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

Thanks!

test/unit_test/static/CMakeLists.txt Outdated Show resolved Hide resolved
@JohelEGP
Copy link
Collaborator Author

JohelEGP commented Mar 18, 2021 •
edited
Loading

All that's left is updating the docs.

+And removing `width{0} / m` too.

mpusz
mpusz approved these changes Mar 18, 2021
View reviewed changes
Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks!!!

@mpusz mpusz merged commit 376c402 into mpusz:master Mar 18, 2021
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
style: mark comment as TODO
dd1a46c 
Addresses #261 (comment).
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
refactor: use values rather than types for reference
1068f7d 
Addresses #261 (comment).
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
refactor: rename unit_constants to references
018c583 
Addresses #261 (comment).
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
refactor!: require explicit number for references
0b87e7c 
Addresses:
- #261 (comment)
- #261 (comment)
- #261 (comment)
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
refactor(test): rename unit_constants.cpp to references.cpp
ed94ba3 
Addresses #261 (comment).
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
refactor(test): rename references.cpp to references_test.cpp
c61b864 
Addresses #261 (comment).
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
refactor: use perfect forwarding for make functions
234337d 
Addresses #261 (comment).
mpusz pushed a commit that referenced this pull request Mar 18, 2021
@JohelEGP @mpusz
refactor: make reference's interface hidden friends
ced4b3b 
Addresses:
- #261 (comment)
- #261 (comment)
@mpusz mpusz added the enhancement New feature or request label Mar 18, 2021
@JohelEGP JohelEGP deleted the reference branch March 18, 2021 21:39
mpusz
mpusz reviewed Mar 18, 2021
View reviewed changes
src/core/include/units/reference.h
Comment on lines +111 to +115
template<QuantityValue Rep>
[[nodiscard]] friend constexpr Quantity auto operator/(const Rep& lhs, reference)
{
return lhs / quantity<D, U, Rep>::one();
}
Copy link
Owner

Choose a reason for hiding this comment

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

I think there is no use for this operator? Probably we should get rid of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess so. Right now, it's a shortcut for 33 / (1 * s).

Copy link
Owner

Choose a reason for hiding this comment

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

In this case I would also prefer to be explicit. With this our * serves exactly as ⋅ in a quantity notation.

Copy link
Owner

Choose a reason for hiding this comment

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

So let's get rid of it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. #262 does that.

JohelEGP added a commit to JohelEGP/units-1 that referenced this pull request Mar 18, 2021
@JohelEGP
refactor: remove operator/(Rep, reference)
8a18f8d 
Addresses mpusz#261 (comment).
mpusz pushed a commit that referenced this pull request Mar 19, 2021
@JohelEGP @mpusz
refactor: remove operator/(Rep, reference)
f0cfc14 
Addresses #261 (comment).
@mpusz mpusz added this to the v0.7.0 milestone May 10, 2021
benediktmoedl pushed a commit to benediktmoedl/mp-units-src that referenced this pull request Sep 4, 2023
@JohelEGP @mpusz
style: mark comment as TODO
f2c1587 
Addresses mpusz/mp-units#261 (comment).
benediktmoedl pushed a commit to benediktmoedl/mp-units-src that referenced this pull request Sep 4, 2023
@JohelEGP @mpusz
refactor: use values rather than types for reference
ef4d0ee 
Addresses mpusz/mp-units#261 (comment).
benediktmoedl pushed a commit to benediktmoedl/mp-units-src that referenced this pull request Sep 4, 2023
@JohelEGP @mpusz
refactor: rename unit_constants to references
89fed6a 
Addresses mpusz/mp-units#261 (comment).
benediktmoedl pushed a commit to benediktmoedl/mp-units-src that referenced this pull request Sep 4, 2023
@JohelEGP @mpusz
refactor!: require explicit number for references
7f2c64b 
Addresses:
- mpusz/mp-units#261 (comment)
- mpusz/mp-units#261 (comment)
- mpusz/mp-units#261 (comment)
benediktmoedl pushed a commit to benediktmoedl/mp-units-src that referenced this pull request Sep 4, 2023
@JohelEGP @mpusz
refactor: use perfect forwarding for make functions
767f57f 
Addresses mpusz/mp-units#261 (comment).
benediktmoedl pushed a commit to benediktmoedl/mp-units-src that referenced this pull request Sep 4, 2023
@JohelEGP @mpusz
refactor: make reference's interface hidden friends
42797b1 
Addresses:
- mpusz/mp-units#261 (comment)
- mpusz/mp-units#261 (comment)
benediktmoedl pushed a commit to benediktmoedl/mp-units-src that referenced this pull request Sep 4, 2023
@JohelEGP @mpusz
refactor: remove operator/(Rep, reference)
bfc14f7 
Addresses mpusz/mp-units#261 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpusz mpusz mpusz approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.7.0
Development

Successfully merging this pull request may close these issues.

refactor!: replace one_rep and unit constants with reference<Dimension, Unit>
2 participants
@JohelEGP @mpusz