Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005775OCamlOCaml generalpublic2012-10-07 02:402012-10-16 11:52
Reporterhongboz 
Assigned Tohongboz 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version4.00.2+dev 
Summary0005775: Bug fixes for tools/pprintast.ml
DescriptionI 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
TagsNo tags attached.
Attached Filespatch file icon pprintast.patch [^] (5,871 bytes) 2012-10-07 08:21 [Show Content]
patch file icon pprintast1.patch [^] (7,349 bytes) 2012-10-11 19:17 [Show Content]

- Relationships

-  Notes
(0008215)
gasche (developer)
2012-10-07 08:20

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.
(0008221)
hongboz (developer)
2012-10-08 00:21

sure, thanks for your new patch
(0008238)
hongboz (developer)
2012-10-10 14:57

lefessan, would you have a look at this patch?
(0008242)
hongboz (developer)
2012-10-11 19:16

I have updated more bug fixes for this file, it was heavily tested
(0008243)
hongboz (developer)
2012-10-11 19:22

gasche, is there any guidelines to commit? I want to read some instructions before I do my first commit.
Many thanks
(0008255)
doligez (administrator)
2012-10-15 14:41

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.
(0008263)
doligez (administrator)
2012-10-16 11:52

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

- Issue History
Date Modified Username Field Change
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
Powered by Mantis Bugtracker