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

Bug fixes for tools/pprintast.ml #5775

Closed
vicuna opened this issue Oct 7, 2012 · 7 comments
Closed

Bug fixes for tools/pprintast.ml #5775

vicuna opened this issue Oct 7, 2012 · 7 comments
Labels

Comments

@vicuna
Copy link

vicuna commented Oct 7, 2012

Original bug ID: 5775
Reporter: @bobzhang
Assigned to: @bobzhang
Status: closed (set by @xavierleroy on 2015-12-11T18:08:15Z)
Resolution: fixed
Priority: normal
Severity: minor
Fixed in version: 4.00.2+dev
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @hcarty

Bug description

I have attached some bug fixes for tools/pprintast.ml, currently I use tools/pprintast.ml to bootstrapping camlP4, if the community reach a consensus that we should have a high quality printer for Parsetree to get valid vanilla ocaml code, I would be happy to spend more time to get a beautiful output
Please review the patch

File attachments

@vicuna
Copy link
Author

vicuna commented Oct 7, 2012

Comment author: @gasche

The patch looks reasonable (but why all those commented lines in the Pcl_fun case?). I haven't looked into it in the details, but given that the original code is quite shaky anyway I think this will be an improvement -- and there are a couple glaring bug fixed by the commit.

As a side remark, could you format your diff in the so-called "unified" format (diff -u)? They're much easier to read online because they provide context around the changes. I think using the unified format is pretty much the standard now, and I will upload a unified version of your patch for future potential reviewers.

@vicuna
Copy link
Author

vicuna commented Oct 7, 2012

Comment author: @bobzhang

sure, thanks for your new patch

@vicuna
Copy link
Author

vicuna commented Oct 10, 2012

Comment author: @bobzhang

lefessan, would you have a look at this patch?

@vicuna
Copy link
Author

vicuna commented Oct 11, 2012

Comment author: @bobzhang

I have updated more bug fixes for this file, it was heavily tested

@vicuna
Copy link
Author

vicuna commented Oct 11, 2012

Comment author: @bobzhang

gasche, is there any guidelines to commit? I want to read some instructions before I do my first commit.
Many thanks

@vicuna
Copy link
Author

vicuna commented Oct 15, 2012

Comment author: @damiendoligez

We don't (yet) have written guidelines. Off the top of my head:

  1. Don't break the build: make sure it compiles before you commit.
  2. Run the (relevant parts of) the testsuite.
  3. Update the Changes file
  4. Increment the "dev" part (and the date) of the VERSION file.

Step (4) is optional for small changes.

Asking for a code review (as you did) is also a good idea, unless it's a small change and you know what you're doing.

@vicuna
Copy link
Author

vicuna commented Oct 16, 2012

Comment author: @damiendoligez

I have committed the second patch (pprintast1.patch) to 4.00 (rev 13015) and trunk (rev 13016).

@vicuna vicuna closed this as completed Dec 11, 2015
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant