replace std:string to absl::string #19032
Conversation
Signed-off-by: Tarun Sharma <[email protected]>
|
/assign-from @envoyproxy/first-pass-reviewers |
|
@envoyproxy/first-pass-reviewers assignee is @dmitri-d |
|
/assign-from @envoyproxy/first-pass-reviewers |
|
@envoyproxy/first-pass-reviewers assignee is @jmarantz |
| @@ -143,7 +143,7 @@ inline void encodeLast(uint64_t pos, uint8_t last_char, std::string& ret, | |||
|
|
|||
| } // namespace | |||
|
|
|||
| std::string Base64::decode(const std::string& input) { | |||
| std::string Base64::decode(absl::string_view input) { | |||
There was a problem hiding this comment.
I think preserving const-ness won't do much harm, but will help to detect erroneous mutation of input inside the function.
| std::string Base64::decode(absl::string_view input) { | |
| std::string Base64::decode(const absl::string_view input) { |
Here and in the other functions.
There was a problem hiding this comment.
I don't feel that strongly about this, but we don't tend to do this from what I've seen in the Envoy codebase. Also why would we do this for absl::string_view and not for (say) int on input params?
There was a problem hiding this comment.
top level const shouldn't be in declaration because it doesn't affect method signature (https://clang.llvm.org/extra/clang-tidy/checks/readability-avoid-const-params-in-decls.html)
but in implementation it is encouraged.
There was a problem hiding this comment.
Thank you @lizan! I stand corrected )
There was a problem hiding this comment.
thank y'all got a great insight!!
There was a problem hiding this comment.
const input tweaks notwithstanding this lgtm.
Also IMO this is not deep enough to require senior maintainer attention but I'll ask for it anyway to adjudicate on const input params :)
/assign-from @envoyproxy/senior-maintainers
While we're waiting for that can you fill out the fields in the PR description, noting "N/A" for fields you feel are not applicable?
Thank you!
|
@envoyproxy/senior-maintainers assignee is @zuercher |
| @@ -143,7 +143,7 @@ inline void encodeLast(uint64_t pos, uint8_t last_char, std::string& ret, | |||
|
|
|||
| } // namespace | |||
|
|
|||
| std::string Base64::decode(const std::string& input) { | |||
| std::string Base64::decode(absl::string_view input) { | |||
There was a problem hiding this comment.
Thank you @lizan! I stand corrected )
Signed-off-by: Tarun Sharma [email protected]
Commit Message: replace std:string to absl::string
Additional Description: #11318
Risk Level: N/A
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
[Optional Runtime guard:] N/A
[Optional Fixes #Issue] N/A
[Optional Fixes commit #PR or SHA] N/A
[Optional Deprecated:] N/A
[Optional API Considerations:] N/A