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

Fix hang when parsing unclosed comment #49

Merged
merged 2 commits into from
Feb 8, 2025

Conversation

cebtenzzre
Copy link
Contributor

Continuation of #37

Since c73b53f, minja hangs when parsing the template {#, e.g.:

#include <minja/minja.hpp>
int main() {
    try {
        minja::Parser::parse("{#", {});
    } catch (...) {
        return 1;
    }
}

This program should exit with status code 1, but instead it hangs.

I found a fix that works even with the nonconforming Microsoft STL, and added a test.

Copy link
Collaborator

@ochafik ochafik left a comment

Choose a reason for hiding this comment

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

Thanks @cebtenzzre !

Trying to understand why tests are all red before this can go in. Some windows failures are clearly unrelated (hendrikmuhs/ccache-action#287), others, not sure yet.

@@ -2336,6 +2337,10 @@ class Parser {
throw std::runtime_error("Unexpected block: " + keyword);
}
} else if (std::regex_search(it, end, match, non_text_open_regex)) {
if (!match.position()) {
assert(match[0] == "{#");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather avoid asserts for now, could you throw something here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. I think some kind of assert macro should be added at some point, though clearly out of scope for this PR.

@@ -1792,7 +1793,7 @@ class Parser {
auto left = parseStringConcat();
if (!left) throw std::runtime_error("Expected left side of 'logical compare' expression");

static std::regex compare_tok(R"(==|!=|<=?|>=?|in\b|is\b|not[\r\n\s]+in\b)");
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍😅 🙈

@cebtenzzre cebtenzzre force-pushed the fix-opencomment-hang branch from e97bb24 to 96ebff4 Compare February 8, 2025 16:16
@cebtenzzre
Copy link
Contributor Author

Is there any reason this repo uses plain merges instead of squash merges for PRs? A linear commit history like llama.cpp has is simpler and keeps fixup commits in PRs out of the main branch.

@ochafik
Copy link
Collaborator

ochafik commented Feb 8, 2025

Is there any reason this repo uses plain merges instead of squash merges for PRs? A linear commit history like llama.cpp has is simpler and keeps fixup commits in PRs out of the main branch.

Main reason has been most of my prs contained sometimes unrelated changes so far (just like this one has the welcome \s simplification ;-)), happy to change going forward.

@ochafik ochafik merged commit 2f27203 into google:main Feb 8, 2025
3 of 11 checks passed
@ochafik
Copy link
Collaborator

ochafik commented Feb 8, 2025

Thanks again @cebtenzzre !

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