-
Notifications
You must be signed in to change notification settings - Fork 20
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
Tolerate missing ';' in message body #218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few remarks, but nothing really blocking except maybe fixing indentation/formatting in proto.y
and maybe TODOs for making the numerous new functions in parser/ast.go
more DRY.
func newMessageElements(semicolons []*ast.RuneNode, elements []ast.MessageElement) []ast.MessageElement { | ||
elems := make([]ast.MessageElement, 0, len(semicolons)+len(elements)) | ||
for _, semicolon := range semicolons { | ||
elems = append(elems, ast.NewEmptyDeclNode(semicolon)) | ||
} | ||
elems = append(elems, elements...) | ||
return elems | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that this and the above functions can be trivially collapsed into a single generic function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is line 141. How do I say T is an interface *ast.EmptyDeclNode implements?
@@ -182,3 +191,12 @@ func toEnumElements[T ast.EnumElement](nodes nodeWithEmptyDecls[T]) []ast.EnumEl | |||
} | |||
return elements | |||
} | |||
|
|||
func toMessageElements[T ast.MessageElement](nodes nodeWithEmptyDecls[T]) []ast.MessageElement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly, we can't make this more generic and collapse into a single function, since there's no way to express assignability/convertibility constraints in Go generic type parameters :/
But we could at least hoist this body into another generic function so each of these functions becomes a one-liner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how to do that.
parser/proto.y
Outdated
@@ -49,7 +45,7 @@ import ( | |||
mtdMsgType *ast.RPCTypeNode | |||
mtdElements []ast.RPCElement | |||
opt *ast.OptionNode | |||
optN nodeWithEmptyDecls[*ast.OptionNode] | |||
optWE nodeWithEmptyDecls[*ast.OptionNode] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "WE" (with empty?) the new notation for "N"? If so, should msgFld
and msgGrp
above instead be fldWE
and grpWE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the opt->optRaw instead (without empty decls is the corner case)
While still producing an error, see: bufbuild#200
While still producing an error, see: #200