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

Require that generator params/args be a superset of typegen params #28

Merged
merged 10 commits into from
Apr 28, 2017

Conversation

hofstee
Copy link
Collaborator

@hofstee hofstee commented Apr 22, 2017

This is a fix for issue #27.

@rdaly525 rdaly525 self-requested a review April 26, 2017 17:29
Copy link
Owner

@rdaly525 rdaly525 left a comment

Choose a reason for hiding this comment

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

This change looks good.

Two fixes:

  1. can you update the checkParams to a name that reflects the operation better?

  2. Can you change this to pull request into the 'dev' branch? I need to do a lot more continuous integration work in order to update master (Because coreIR is a dependency on another larger Stanford project)

@hofstee hofstee changed the base branch from master to dev April 26, 2017 17:37
hofstee and others added 2 commits April 26, 2017 13:55
@hofstee
Copy link
Collaborator Author

hofstee commented Apr 26, 2017

Retargeted onto dev.

@hofstee
Copy link
Collaborator Author

hofstee commented Apr 26, 2017

I think checkArgs might be a better name than checkParams, but I'm not sure if that's the best name for this.

@hofstee hofstee requested a review from rdaly525 April 27, 2017 02:22
Copy link
Owner

@rdaly525 rdaly525 left a comment

Choose a reason for hiding this comment

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

@THofstee Super sorry about this... There are a bunch of changes that are merged that probably are inconsistent with some of the things you did. Let me know if I could help fix anything.

Also I think I need two functions for the checkParams thing. One that checks if they are exactly equal and one if the params are a superset of the args.

Maybe the names areArgsParamsMatching, and areArgsParamsSubset. Or something along those lines. I am terrible at coming up with names. Ideally the name is some sort of question that describes the boolean operation explicitly.

@hofstee
Copy link
Collaborator Author

hofstee commented Apr 28, 2017

Out of curiosity, what would be the reasoning for having another function test whether or not they are exactly equal?

@rdaly525
Copy link
Owner

I do the matching check on the Args being passed into the generator. Is there a good situation where you would want to pass in more args to a generator instance than the generator needed? Perhaps?
I do totally agree that the typegen should be a subset of the generator args.

@hofstee
Copy link
Collaborator Author

hofstee commented Apr 28, 2017

I'm not sure. Previously in the other branch I was working in I used it out of convenience because I didn't need a type to pass into runGenerator, but I would get a segfault if I passed in a nullptr so I just grabbed the TypeGen for the Generator and ran it with the arguments for the generator.

I think it's probably better to only have the equality check instead of subset when you provide arguments to the parameters. I can't think of any programming languages that allow you to pass in more arguments to a function than the function has parameters, and I can imagine a lot of nasty bugs that might result from this.

I'll need to double check to see if the nullptr passed to runGenerator still causes a segfault in the dev branch. I'll clean this up a bit tomorrow.

@rdaly525
Copy link
Owner

Just as a warning, I did clean up the infrastructure for running the typegen and generator functions a bit. I made them into an actual method of the class rather than a random function (although I did keep that ability to pass in a random function). If the API is not clear, definitely let me know and add it to Issue #33. Also if you are getting segfaults, then definitely report an issue. Ideally you should not be able to segfault.

@rdaly525
Copy link
Owner

Thanks again for looking into this stuff! I know it is annoying that things keeps changing, but that is the definition of Agile!

@hofstee
Copy link
Collaborator Author

hofstee commented Apr 28, 2017

Seeing as how there is only one function now to check that args exactly match params, I've just left it renamed checkArgs similar to other compilers.

@hofstee hofstee requested a review from rdaly525 April 28, 2017 18:31
Copy link
Owner

@rdaly525 rdaly525 left a comment

Choose a reason for hiding this comment

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

Looks good!

@rdaly525 rdaly525 merged commit 820fbc8 into rdaly525:dev Apr 28, 2017
@hofstee hofstee deleted the typegen-args branch April 28, 2017 21:09
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