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

Warn about leftover merge conflict markers #1550

Closed
wants to merge 1 commit into from

Conversation

Rangi42
Copy link
Contributor

@Rangi42 Rangi42 commented Oct 22, 2024

No description provided.

@Rangi42 Rangi42 added enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM labels Oct 22, 2024
@Rangi42 Rangi42 added this to the v0.9.0 milestone Oct 22, 2024
@Rangi42 Rangi42 requested a review from ISSOtm October 22, 2024 18:57
Comment on lines +424 to +434
OP_SHL OP_SHL OP_SHL OP_LOGICLT {
::error(
"syntax error, unexpected <<<<<<< line (is it a leftover merge conflict mark?)\n"
);
}
| OP_USHR OP_USHR OP_LOGICGT {
::error(
"syntax error, unexpected >>>>>>> line (is it a leftover merge conflict mark?)\n"
);
}
| OP_LOGICEQU OP_LOGICEQU OP_LOGICEQU POP_EQUAL {
Copy link
Member

Choose a reason for hiding this comment

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

Not super convinced, because 1. this enables extra whitespace between the tokens (e.g. << << << <, but inconsistently, not >> >> >> >), and 2. there may be more lex+syntax errors from the rest of the line (e.g. >>>>>>> 0abcdef (Quack I'm a c🦆mmit message) will throw parser errors and even a lexer one on the duck).

Proper handling would, I believe, require emitting those as separate lexer tokens (which is annoying because their length means backtracking... unless we treat all “intermediate” tokens, such as >>>>, as something like idk “truncated merge conflict marker”?)

Further, there is a small UX deficiency in that we will process all 2+ “merge branches”, which is likely to create spurious “duplicate definition” errors or similar, and thus drown out the actual error. Ideally we'd pick a single one (the first one, for simplicity?) and process it and only it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are very good points. I'd prefer not to define large new tokens just for the sake of a rarely-occurring third-"party" specialized error message, and would also not want special logic to pick one "branch" of a merge conflict (what if the conflict "syntax" is wrong, i.e. non-matching <<<<<<< ... ======= ... >>>>>>> delimiters?). Given that, let's not bother being extra-convenient in this case, unless it becomes more practical to do in the 🦀 rewrite.

@Rangi42 Rangi42 closed this Oct 22, 2024
@Rangi42 Rangi42 deleted the merge-conflicts branch October 22, 2024 19:55
@Rangi42 Rangi42 removed this from the v0.9.0 milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Typically new features; lesser priority than bugs rgbasm This affects RGBASM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants