|Anonymous | Login | Signup for a new account||2013-05-21 02:48 CEST|
|Main | My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0005775||OCaml||OCaml general||public||2012-10-07 02:40||2012-10-16 11:52|
|Priority||normal||Severity||minor||Reproducibility||have not tried|
|Target Version||Fixed in Version||4.00.2+dev|
|Summary||0005775: Bug fixes for tools/pprintast.ml|
|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
|Tags||No tags attached.|
|Attached Files|| pprintast.patch [^] (5,871 bytes) 2012-10-07 08:21 [Show Content]
pprintast1.patch [^] (7,349 bytes) 2012-10-11 19:17 [Show Content]
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.
|sure, thanks for your new patch|
|lefessan, would you have a look at this patch?|
|I have updated more bug fixes for this file, it was heavily tested|
gasche, is there any guidelines to commit? I want to read some instructions before I do my first commit.
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.
|I have committed the second patch (pprintast1.patch) to 4.00 (rev 13015) and trunk (rev 13016).|
|2012-10-07 02:40||hongboz||New Issue|
|2012-10-07 02:40||hongboz||File Added: pprintast.patch|
|2012-10-07 08:20||gasche||Note Added: 0008215|
|2012-10-07 08:21||gasche||File Deleted: pprintast.patch|
|2012-10-07 08:21||gasche||File Added: pprintast.patch|
|2012-10-08 00:21||hongboz||Note Added: 0008221|
|2012-10-10 14:57||hongboz||Note Added: 0008238|
|2012-10-10 14:58||hongboz||Assigned To||=> lefessan|
|2012-10-10 14:58||hongboz||Status||new => assigned|
|2012-10-11 19:16||hongboz||Note Added: 0008242|
|2012-10-11 19:17||hongboz||File Added: pprintast1.patch|
|2012-10-11 19:22||hongboz||Note Added: 0008243|
|2012-10-11 19:23||hongboz||Assigned To||lefessan => hongboz|
|2012-10-11 19:23||hongboz||Severity||tweak => minor|
|2012-10-15 14:41||doligez||Note Added: 0008255|
|2012-10-16 11:52||doligez||Note Added: 0008263|
|2012-10-16 11:52||doligez||Status||assigned => resolved|
|2012-10-16 11:52||doligez||Resolution||open => fixed|
|2012-10-16 11:52||doligez||Fixed in Version||=> 4.00.2+dev|
|Copyright © 2000 - 2011 MantisBT Group|