-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[sap] Use C++20 'requires' syntax for double-only functions #21440
[sap] Use C++20 'requires' syntax for double-only functions #21440
Conversation
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.
Thank you so much @jwnimmer-tri! this is definitely a useful C++20 fature, making the intetion very clear.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers
multibody/contact_solvers/sap/sap_solver.h
line 306 at r1 (raw file):
// @pre context is not nullptr. // @pre context was created via a call to model.MakeContext(). SapSolverStatus SolveWithGuessImpl(const SapModel<T>& model,
btw, I like this function name change!
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.
adding myself +@amcastro-tri for feature review
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers
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.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers
multibody/contact_solvers/sap/sap_solver.cc
line 218 at r1 (raw file):
systems::Context<T>* context) requires std::is_same_v<T, double> { // NOLINT(whitespace/braces)
BTW What's the story with this linting directive? Is this going to have to be part of every incantation that uses requires
?
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.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers
multibody/contact_solvers/sap/sap_solver.cc
line 218 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW What's the story with this linting directive? Is this going to have to be part of every incantation that uses
requires
?
As of today, yes. The medium-term goal is to either:
(1) Change our linters so that when enable_clang_format_lint = True
, the whitespace/*
rules in CPPLINT are disarmed, or
(2) Upgrade to a newer cpplint that understands requires
.
FYI We'll give this until my afternoon time for any platform reviewers to raise objections (per my slack post), but then we can merge it. |
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees amcastro-tri,SeanCurtis-TRI(platform)
This change is