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

Pyparsing extra debug information on parse failure and preliminary macro support #34

Merged
merged 19 commits into from Apr 12, 2012

Conversation

pwaller
Copy link
Contributor

@pwaller pwaller commented Apr 7, 2012

.. by preliminary I mean very preliminary. They are sort of parsed, but then ignored at code generation for the moment.

  • Parser needs to also parse arguments to the macro
  • Then these arguments need to be usable within the macro blocks, within opcodes
  • Macros need to be callable somehow (not sure of syntax for this)

The extra debugging information should help in case of parse failures. Now it prints the line number in a friendly way and points at the point of parse failure rather than excepting and bailing out.

@jtauber
Copy link
Owner

jtauber commented Apr 7, 2012

Should this still be considered a work in progress? if so, let's change the heading to include [IN PROGRESS]

Perhaps we should split the macro support into its own pull request. I don't like having two orthogonal things in a pull request.

Also, I'm wondering the best place to report asm_pyparsing issues back to you; the GitHub issues on your fork? In particular asm_pyparsing isn't doing DAT of strings correctly (asm.py now supports them)

@swarmer
Copy link
Contributor

swarmer commented Apr 7, 2012

I think all discussion should be in pull requests that are opened early,
discussed during active development and merged when people think it's the time.

@pwaller
Copy link
Contributor Author

pwaller commented Apr 7, 2012

@jtauber, point taken about putting stuff into different pull requests. In this case, it didn't seem to me to be worth the overhead of making two separate ones, and the development was done together. All of the changes are more or less incremental improvements, and I wanted to make the work in progress on macro support visible.

With regards to the issues, I'm fine with them going here if you're okay with that, though it's best to @mention me if you want me to notice for the first time so that I get a notification.

@swarmer - do you know of a way to add code to a pull request once it is made?

@swarmer
Copy link
Contributor

swarmer commented Apr 7, 2012

Um, you just push the code? I think you can even open request again with new commits after it was merged.

@jtauber
Copy link
Owner

jtauber commented Apr 7, 2012

@pwaller if you just continue to push to the branch, it adds it to the pull request

@pwaller
Copy link
Contributor Author

pwaller commented Apr 7, 2012

Macros almost work, but not quite. I'm just having trouble figuring out how to modify the macro so that arguments are substituted. It shouldn't be much more work, but I don't have time for it in the immediate future.

@pwaller
Copy link
Contributor Author

pwaller commented Apr 7, 2012

Also just to mention that I've been working on making the parser a bit more robust, so this should catch a fair number of mistakes and tell you where they are, and developing on the grammar is easier with the new debugging information.

@swarmer
Copy link
Contributor

swarmer commented Apr 8, 2012

If this branch is not going to be merged soon I suggest to backport 4358c1f into master
because I wasn't able to assemble hello2.asm successfully without this commit cherry-picked.

@andre-d
Copy link
Contributor

andre-d commented Apr 8, 2012

I agree with @swarmer. Also, @pwaller how do you feel about using single quotes for packed 8bit byte data?

@jtauber
Copy link
Owner

jtauber commented Apr 8, 2012

If someone submits a separate pull request to backport 4358c1f I'll accept it

@pwaller
Copy link
Contributor Author

pwaller commented Apr 9, 2012

So far as I can tell, macro support now works properly, including things like labels inside macros.

It turned out to be rather more complicated to implement than I had hoped. Apologies for putting so much in one pull request but it turned out to be a fairly major re-architecting and it would have been a lot of overhead to break it up.

@pwaller
Copy link
Contributor Author

pwaller commented Apr 9, 2012

@andre-d re: single quotes. Not sure how I feel about introducing an unofficial syntax. Let's cope without for the moment it can be a new issue.

@andre-d
Copy link
Contributor

andre-d commented Apr 11, 2012

Is this pull request meant to be finished? Your title still says IN PROGRESS

@pwaller
Copy link
Contributor Author

pwaller commented Apr 11, 2012

Macros now work, so I guess this can be merged. I'm not entirely happy with it, but I don't see going back either, since there are quite a few fixes in here.

jtauber added a commit that referenced this pull request Apr 12, 2012
Pyparsing extra debug information on parse failure and preliminary macro support
@jtauber jtauber merged commit c6c19dc into jtauber:master Apr 12, 2012
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.

None yet

4 participants