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

Added annotations to individual fields in a struct #347

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

Conversation

aiverson
Copy link
Contributor

This is an initial implementation of #324

Only the basic syntax is implemented, so using commas to chain annotations doesn't yet work.

@aiverson aiverson changed the base branch from develop to master February 12, 2019 23:56
@aiverson
Copy link
Contributor Author

Oh, terralist isn't in master yet?

@aiverson
Copy link
Contributor Author

It looks like everything breaks when I try to port it to master.

@elliottslaughter
Copy link
Member

Did develop add a filter method to lists, or something?

annotatedstruct.t:
annotatedstruct.t:72: attempt to call method 'filter' (a nil value)
stack traceback:
	annotatedstruct.t:72: in function 'userfn'
	annotatedstruct.t:72: Errors reported during evaluating Lua code from Terra
        [class.entries:filter(
             ^
	src/terralib.lua:1801: in function 'evalluaexpression'
	src/terralib.lua:2825: in function 'docheck'
	src/terralib.lua:3036: in function 'checksingle'
	src/terralib.lua:3238: in function 'checkstmts'
	src/terralib.lua:3132: in function 'checkblock'
	src/terralib.lua:3322: in function 'typecheck'
	src/terralib.lua:1129: in function 'defineobjects'
	annotatedstruct.t:71: in function 'userfn'
	annotatedstruct.t:80: Errors reported during invoking metatype function

@aiverson
Copy link
Contributor Author

aiverson commented Feb 13, 2019

Yeah, develop added filter and reduce methods to lists. They are quite handy and I am in favor of bringing all of the terralist changes back to master. It moved twice between master and develop. List was originally a small section in asdl.lua, but then it got moved to list.lua and then terralist.lua

@elliottslaughter
Copy link
Member

I'm fine with bringing that over, but just FYI, develop did a bunch of stuff with the way that lua files get packaged into the terra binary---which was mostly all for PUC Lua support and ended up being completely unnecessary in the new version of that patch. So watch out for changes there if you're bring it over; we don't want to bring along the entire set of changes to internalized files from the develop branch.

@aiverson
Copy link
Contributor Author

This should be ready to merge now unless anyone sees an issue.

@elliottslaughter
Copy link
Member

Were the whitespace changes isolated in a single commit? We've got some pending PR's that I'd be slightly concerned about having merge conflicts with, and at least one of those (PUC Lua support) won't be ready to go any time soon.

Probably what I should aim to do is having a separate PR for the whitespace, and then we can have a well-defined point at which we can expect the code to be clean for this.

@aiverson
Copy link
Contributor Author

No, there were a couple of commits. I agree that we should fix all the whitespace issues. We should probably make a .editorconfig file and have code style guidelines

@elliottslaughter
Copy link
Member

Is there a good way in git to apply a cosmetic change to multiple branches simultaneously, so that after the change you don't get merge conflicts?

Personally, I've been wanting to use clang-format on the source code, since it's not just the whitespace that's inconsistent, but I've been holding off because we continue to have long-term branches that would have massive merge conflicts if we did.

@aiverson
Copy link
Contributor Author

Not that I know of.

@elliottslaughter
Copy link
Member

I'm planning to take #352 and #349, then we can rebase/merge this on master. At that point the big outstanding change will be #320, but that could probably be manually merged against these changes.

@aiverson
Copy link
Contributor Author

Now that the other things are merged, can this be merged too?

@elliottslaughter
Copy link
Member

Did we reach consensus on the syntax in #324? It looked like there was a new round of discussion there, and I'm not entirely caught up yet.

@elliottslaughter
Copy link
Member

Can we split the last commit into a separate PR, and also make sure we have a test case for mutable iterators? Thanks.

@aiverson
Copy link
Contributor Author

Yeah... Sorry, my branches got a bit jumbled. I didn't expect to start working on other features before the first annotations draft got merged.

@elliottslaughter
Copy link
Member

I rebased this onto master at https://github.com/elliottslaughter/terra/tree/pr-347-rebase

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