feat(opt): emit rule comments as options - #110
Merged
Merged
Conversation
A `//` comment is an option, as in FreeBSD where it is an O_NOP opcode among the rule options. The parser hands it to State.OnOption as an OptComment holding the text up to any `#`, wherever it stands: after a body, after check-state or as the body of a comment-only rule. It takes the rest of the line, so inside a group it leaves the group unclosed, and `not //` is accepted and makes the rule never match. ParseOptions consumes the comment instead of stopping at it. The VM drops a comment that decides nothing when it builds a rule, so a commented rule keeps its empty option run. This breaks the API: Instruction.InlineComment is gone, the comment arrives as an option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
//comment is a rule option, as in FreeBSD, whereTOK_COMMENTsits amongrule_optionsandfill_commentwrites anO_NOPopcode that keeps itsF_NOTandF_ORbits. The parser now hands it toState.OnOptionas anOptCommentwhoseTextis the rest of the line up to any#, leading space kept and trailing whitespace removed, instead of storing it inInstruction.InlineComment.The comment arrives wherever it stands: after a rule body, after
check-state, and as the body of a comment-only rule (add 100 // notestill gives a count rule withTargetAnysource and destination, then the comment option).ParseOptionsconsumes it instead of stopping at it, and anOptionHookis never asked about//.Taking the rest of the line, a comment inside
{ … }leaves the group unclosed:{ in or // c }now fails withErrExpectedOrat the end of the line, where it used to beErrUnknownOption.not // textis accepted, as upstream does, and makes the rule never match.VM
program.DropCommentsremoves a comment that decides nothing before a rule is closed: not negated, not in an or-group and not followed by an option joined to it. A commented rule therefore keeps an empty option run and the fast path of a rule without options. A negated comment, or one a hook placed in a group, stays and holds throughmatchPolicy, sonot //fails its rule.Unchanged divergences
The accepted syntax is otherwise the same as before. FreeBSD requires
//to be a whole word, here glued forms such asin// ckeep being accepted. The 80-character limit is not enforced and the words are not rejoined with single spaces.Compatibility
Breaking:
Instruction.InlineCommentis removed, the comment is an option. v0.2.0 is already incompatible with v0.1.0.Performance
taskset -c 12-15, prebuilt binaries before and after,benchstat, n=6:Parser_Next_CommentOnlyRuleParser_Next_CommentOptionLong(about 4 KB of comment)Parser_Next_OptionsAfterTargetParser_Next_Grammar/Ruleset/RawA comment line now pays for a 192-byte
Optthrough theStateinterface. A comment right after the destination skips the speculative option pass, without which the long comment was 44% slower. AllBenchmark_Parser_Next*stay at 0 allocs/op, and the VM check path keeps itsAllocsPerRun == 0guard.Validation
Tests were rewritten first and failed: every
InlineCommentassertion became a fullReduceStatewith theOptCommentoption, plus new cases for CRLF,not //, comments inside groups, a state rejecting the comment, hooks next to comments, andTest_VM_Check_CommentOptionfor the verdicts.make test(race) andmake lintpass.