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

cleaned up handling of specifying multiple libraries at once #6593

Merged
merged 4 commits into from
Jul 16, 2024

Conversation

firewave
Copy link
Collaborator

No description provided.

@firewave
Copy link
Collaborator Author

The GUI only passes a single file to the library loading.

@@ -147,3 +148,21 @@ void findAndReplace(std::string &source, const std::string &searchFor, const std
index += replaceWith.length();
}
}

std::list<std::string> splitString(const std::string& str, char sep)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's bikeshed about this unimportant utility function, which seems quite inefficient.

We could use a very short regex version (which also doesn't produce empty strings), or this replacement:

std::list<std::string> splitString2(const std::string& str, char sep)
{
    std::list<std::string> l;
    std::string::size_type pos = 0;
    for (;;) {
        const std::string::size_type newPos = str.find(sep, pos);
        l.push_back(str.substr(pos, newPos - pos));
        if (newPos == std::string::npos) {
            break;
        }
        pos = newPos + 1;
    }
    return l;
}

https://godbolt.org/z/85E8xGThd

Copy link
Collaborator Author

@firewave firewave Jul 15, 2024

Choose a reason for hiding this comment

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

I already a simpler version locally (which might be identical to your version) which I will add in a follow-up PR which will also extend its usage. I wanted to keep this topical and not introduce any functional non-refactoring changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see.
There still is a merge conflict.

@chrchr-github chrchr-github merged commit c7bc7a8 into danmar:main Jul 16, 2024
63 checks passed
@firewave firewave deleted the lookup-lib-multi branch July 16, 2024 08:38
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