Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Generic var templates for operators and std::iterator_trait var/fvar specialization #1525
Generic var templates for operators and std::iterator_trait var/fvar specialization #1525
Changes from all commits
8309363
bd9b20c
f8f2e3a
8806331
a6b4d81
3868acf
5e07ef9
119a7e7
1348dda
858e1f2
ec6345a
4300497
fd35443
73181b4
36777ab
ad62f4f
fd2a4a8
930329d
6185370
12ec611
88c86e8
5564039
a694d1a
5740cd4
4cba532
30ee8c9
1a565b6
75eaf43
1342d33
3604151
9eebaa7
993ceaa
cd60021
ced2df7
8d460ac
2795719
99a916b
84f5b26
06ad089
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[comment]
These are all super useful. Thanks for adding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly prefer
removing the
require_
prefix because that's implying a use, not a behaviorreplace
_t
with_v
because it denotes a boolean value, not a type; this follows the behavior ofis_arithmetic_v
;enable_if_t
ends in_t
because it denotes a type (void
by default), not a value.For example, that'd be
not_autodiff_v<T>
fortrue
ifT
is not an autodiff type, and usage would be, for exampleenable_if_t<not_autodiff_v<T>, U>
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify? I'm not sure i understand how the require part of the name is implying a use over a behaviour. The intent with the
require_
in the name is because all thoserequire_*
s are trying to emulate a legacy version of C++20 concepts. It's for if you want a universal reference in the function signature for all the weird value types and const combinations. In terms of use/behavior I think my answer is that I think about C++ concepts like compile time contracts (which feels like a behaviour, though idk what that word means in context here)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
require_*_t
metaprogramming stuff is essentially a convoluted / compact way to writestd::enable_if_t<...>
i.e. they also returnvoid
.You can think of
require_autodiff_t
as an alias forenable_if_t<not_autodiff_v<T>, U>
.Also idt we can use C++ compile time constexpr objects with c++1y (anything _v like how std does) since the windows compiler we require compatibility with for Rtools has a bug for those
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I misunderstood and thought this was a boolean. Now that I see what it's doing, I still think it needs to be renamed. How about
enable_if_autodiff_t
to enable if it's an autodiff type? That follows the standard library naming conventions more closely thanrequire_autodiff_t
.We can then use
disable_if_autodiff_t
instead ofenable_if_not_autodiff_t
.We can build similar ones for primitives,
enable_if_arithmetic_t
anddisable_if_arithmetic_t
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these all have doc or are they so close to the ones without
_v
that they're obvious? cppreference actually doesn't doc these and they doc everything else meticulously.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been throwing this back and forth with myself. I've leaned towards not doc since aliases are inlined in the docs you can see the full definition
http://mc-stan.org/math/d0/dfb/group__require__base__types.html#gac55eae5c1fc2f38a96336b5f0bc94449