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

Report entity name to SDT function #85

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

onokonem
Copy link

@onokonem onokonem commented Mar 2, 2019

wil be replaces by the name of the entity on preparing SDT call

Copy link
Collaborator

@shivansh shivansh left a comment

Choose a reason for hiding this comment

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

This looks great! Please address the comments below.

@@ -91,33 +91,14 @@ func (T *Token) UintValue() (uint64, error) {
return strconv.ParseUint(string(T.Lit), 10, 64)
}

var (
Copy link
Collaborator

@shivansh shivansh Mar 7, 2019

Choose a reason for hiding this comment

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

I'm not sure but are the variables holding compiled regular expressions required to be global ?

Copy link
Author

Choose a reason for hiding this comment

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

of course it is not required to be global

in the same time it might be compiled once on package init and used everywhere later

sdtPlaceholders = regexp.MustCompile(`\$(\d+)`)
sdtReplacement = []byte("X[$1]")
)

Copy link
Collaborator

@shivansh shivansh Mar 7, 2019

Choose a reason for hiding this comment

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

I think a small comment here explaining the functionality of SDTVal() would be helpful in future. Something along the lines of -

// SDTVal converts a handwritten action expression into an equivalent expression
// usable by gocc during runtime.
// For e.g. the expression `<< $0.(int64) + $2.(int64), nil >>` is converted to
// `X[0].(int64) + X[2].(int64), nil`. `X` usually represents a slice of
// attributes in the code generated by gocc.

Copy link
Author

Choose a reason for hiding this comment

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

there was no such explanation in the original code, so I do did not add it :)

are you sure any explanation is required here? this code is completely self-explaining FMPOV

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