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

Add lower and upper string operations to stringify header. #4933

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

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Sep 30, 2024

No description provided.

@asl
Copy link
Contributor

asl commented Sep 30, 2024

There are AsciiStrToUpper and AsciiStrToLower in abseil. See https://github.com/abseil/abseil-cpp/blob/master/absl/strings/ascii.h#L207 and around

Copy link
Contributor

@vlstill vlstill left a comment

Choose a reason for hiding this comment

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

Thanks for this and for putting it to a separate PR.

lib/stringify.h Outdated

/// Convert a string to upper case. This is not safe for non-ASCII strings.
/// Performs the up operation in place.
void upperinPlace(std::string &s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void upperinPlace(std::string &s);
void upperInPlace(std::string &s);

lib/stringify.h Outdated
@@ -123,6 +123,21 @@ cstring toString(const void *value);

char DigitToChar(int digit);

/// Convert a string to lower case. This is not safe for non-ASCII strings.
/// Performs the lowering operation in place.
void lowerinPlace(std::string &s);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
void lowerinPlace(std::string &s);
void lowerInPlace(std::string &s);

lib/stringify.h Outdated
/// Performs the lowering operation in place.
void lowerinPlace(std::string &s);

/// Convert a string to lower case. This is not safe for non-ASCII strings.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is (probably) "safe" in a sense it will not crash. It is just not what might be expected (e.g. locale-aware UTF-8 lowercase).

@fruffy
Copy link
Collaborator Author

fruffy commented Sep 30, 2024

There are AsciiStrToUpper and AsciiStrToLower in abseil. See https://github.com/abseil/abseil-cpp/blob/master/absl/strings/ascii.h#L207 and around

I always forget we have Abseil now.. I will use these instead of transform then

@fruffy fruffy force-pushed the fruffy/string_utils branch 2 times, most recently from 0a134e4 to 53ea8d7 Compare September 30, 2024 17:48
Signed-off-by: fruffy <fruffy@nyu.edu>
@fruffy fruffy force-pushed the fruffy/string_utils branch from 53ea8d7 to 217816f Compare September 30, 2024 18:08
@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Sep 30, 2024
@fruffy
Copy link
Collaborator Author

fruffy commented Oct 24, 2024

The test fails for non-ascii characters. I will wait for C++20, maybe there is something that can be done here then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants