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

Refactor gemma/common.cc to improve readability and safety #458

Closed
wants to merge 1 commit into from

Conversation

ericcurtin
Copy link

Use std::size for array size calculations. Replace C-style string manipulations with std::string methods. Simplify std::transform usage for case conversion.

@ericcurtin ericcurtin force-pushed the common branch 3 times, most recently from 8c2502b to e01bcd2 Compare December 8, 2024 21:23
jan-wassenberg
jan-wassenberg previously approved these changes Dec 9, 2024
Copy link
Member

@jan-wassenberg jan-wassenberg left a comment

Choose a reason for hiding this comment

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

Nice, thanks for making the change :)

@jan-wassenberg jan-wassenberg added the copybara-import Trigger Copybara for merging pull requests label Dec 9, 2024
@jan-wassenberg
Copy link
Member

Apologies, I didn't see this PR is targeting main branch. Our import infra does not support that. Please change to dev branch :)

Use `std::size` for array size calculations. Replace C-style
string manipulations with `std::string` methods. Simplify
`std::transform` usage for case conversion.

Signed-off-by: Eric Curtin <[email protected]>
@ericcurtin ericcurtin changed the title Refactor gemma/common.cc to improve readability and safety: Refactor gemma/common.cc to improve readability and safety Dec 9, 2024
@ericcurtin ericcurtin changed the base branch from main to dev December 9, 2024 16:02
@ericcurtin ericcurtin dismissed jan-wassenberg’s stale review December 9, 2024 16:02

The base branch was changed.

@jan-wassenberg jan-wassenberg removed the copybara-import Trigger Copybara for merging pull requests label Dec 9, 2024
@ericcurtin
Copy link
Author

Thanks, it's against dev now

@jan-wassenberg
Copy link
Member

Thanks for rebasing :) I think the email address used is not covered by CLA?

@ericcurtin
Copy link
Author

Author: @a-googler <no****ly​@google.com>

apparent didnt sign the CLA, dunno what that means :)

@jan-wassenberg
Copy link
Member

Oh, I see. No idea how that a-googler got into the check. It might help to close this PR and open a new one? Sorry about the incidental complexity :(

@ericcurtin
Copy link
Author

No problem 😄

@ericcurtin ericcurtin closed this Dec 9, 2024
@ericcurtin
Copy link
Author

New PR:

#460

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants