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

Fast path for handling fixed length strings? #6846

Closed
Andersama opened this issue Sep 20, 2023 · 6 comments
Closed

Fast path for handling fixed length strings? #6846

Andersama opened this issue Sep 20, 2023 · 6 comments

Comments

@Andersama
Copy link

Was looking at text rendering code and I spotted that "%s" is specifically tested likely as a fast path for formatting text.

void ImGui::TextColoredV(const ImVec4& col, const char* fmt, va_list args)
{
    PushStyleColor(ImGuiCol_Text, col);
    if (fmt[0] == '%' && fmt[1] == 's' && fmt[2] == 0)
        TextEx(va_arg(args, const char*), NULL, ImGuiTextFlags_NoWidthForLargeClippedText); // Skip formatting
    else
        TextV(fmt, args);
    PopStyleColor();
}

Was wondering if it may be worth having a fast path for the "%.*s" format. If I remember correctly the format uses an int argument first followed by a char pointer, so not that much different from "%s". I don't use c variadics much, I'm assuming something like the following works:

void ImGui::TextColoredV(const ImVec4& col, const char* fmt, va_list args)
{
    PushStyleColor(ImGuiCol_Text, col);
    if (fmt[0] == '%' && fmt[1] == 's' && fmt[2] == 0)
        TextEx(va_arg(args, const char*), NULL, ImGuiTextFlags_NoWidthForLargeClippedText); // Skip formatting
    else if (fmt[0] == '%' && fmt[1] == '.' && fmt[2] == '*' && fmt[3] == 's' && fmt[4] == 0)
       TextEx(va_arg(args, int), va_arg(args, const char*), ImGuiTextFlags_NoWidthForLargeClippedText); // Skip formatting
    else
        TextV(fmt, args);
    PopStyleColor();
}
@ocornut
Copy link
Owner

ocornut commented Sep 21, 2023

It's possible indeed and may be worthy as we transition to string view branch.
Curious if you actually need/use it or if you suggested this without a strong need?

@Andersama
Copy link
Author

Andersama commented Sep 21, 2023

It was a passing thought, actually while I was looking at this earlier is there any concern about accessing memory out of bounds with just const char* fmt?

Presumably like with other functions the format string would also be a good thing to know the length of.

Working on a text editor as a personal project.

@PathogenDavid
Copy link
Contributor

is there any concern about accessing memory out of bounds

Since fmt is null-terminated it should always be at least one char.

For an empty string (IE: fmt = { '\0' }), the fmt[0] == '%' condition will be in-bounds and result in false, which will short-circuit/skip the other checks (which would've been out-of-bounds.)

@Andersama
Copy link
Author

I suppose my gut instinct is because there's a missing null check that something would go wrong, but that's a different out of bounds condition than the one I was thinking of, so I almost feel silly asking.

@PathogenDavid
Copy link
Contributor

No worries, we all get brain farts.

Dear ImGui is old school and generally doesn't explicitly check for basic mistakes like passing null for something that should never be null.

ocornut added a commit that referenced this issue Sep 25, 2023
@ocornut
Copy link
Owner

ocornut commented Sep 25, 2023

Pushed this change with d486920
Thanksfully the code has been massaged (you are on an older version) so there's only 1 spot to modify now.
I don't feel this is very important but as we'll aim to better support languages which might encourage users to perform formatting on their side, this seems to make sense going ahead.

@ocornut ocornut closed this as completed Sep 25, 2023
ocornut added a commit that referenced this issue Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants