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

[wpilib] Add SmartDashboard::PutData() reference overload #7361

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

calcmogul
Copy link
Member

No description provided.

@calcmogul calcmogul requested a review from a team as a code owner November 8, 2024 05:09
@PeterJohnson
Copy link
Member

Historically we used a pointer here to indicate that the lifetime of the passed-in object mattered (e.g. because we store that pointer and call it later). It makes it at least somewhat more obvious that you should think about the lifetime of the thing you're passing (e.g. you shouldn't pass a local variable). What's the advantage of having the reference overload?

@calcmogul
Copy link
Member Author

calcmogul commented Nov 8, 2024

Historically we used a pointer here to indicate that the lifetime of the passed-in object mattered

I highly doubt our target audience is picking up on that nuance, especially since we don't explain that convention anywhere.

What's the advantage of having the reference overload?

The same advantages of providing reference overloads for constructors: it's more idiomatic, and our users don't have to remember to spam prefix ampersands to get their code to compile.

@calcmogul calcmogul force-pushed the wpilib-add-smartdashboard-overload branch from 857c3d8 to df5ba46 Compare November 9, 2024 04:46
@spacey-sooty
Copy link
Contributor

spacey-sooty commented Dec 16, 2024

It makes it at least somewhat more obvious that you should think about the lifetime of the thing you're passing (e.g. you shouldn't pass a local variable).

To you or another WPILib maintainer? Sure. To a 14 year old who learnt C++ two weeks ago? Hell no. We were a C++ team until recently and as much as we tried to teach students about pointers and how memory worked it's a difficult concept to get through to people, they won't get this nuance they'll just do whatever they can to make their code compile (so just add an ampersand and never grasp the meaning of it).

I think this PR is fine as is itd just make students lives a bit easier, it doesn't allow them to make a mistake they couldn't before.

@calcmogul calcmogul force-pushed the wpilib-add-smartdashboard-overload branch from df5ba46 to 663bf80 Compare December 22, 2024 21:44
@github-actions github-actions bot added the component: wpilibc WPILib C++ label Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants