Comment author: @bobzhang

IIRC, this bug was already fixed in trunk. The tools/pprintast.ml was quite primitive(a lot of corner cases are not covered, and the indentation was not good).
please don't use it for production, I did a rewrite for this file, when it's ready I will commit it , thanks

Comment author: turpin

Indeed, it is fixed: no pore pprintast.ml in the trunk ;-)

Comment author: @alainfrisch

Tiphaine: pprintast.ml is now in parsing/, no longer in tools/.

Comment author: @bobzhang

Alain, actually I am ready, but found some problems so that I was waiting damien to help, but you can rename AstPrint.ml into pprintast.ml and commit if you like. Now, the output is quite good, bug report is welcome ;-)

Comment author: @alainfrisch

Hongbo: feel free to commit your patch directly.

Comment author: @bobzhang

turpin, I've committed a new parsing/pprintast.ml in the trunk, bug report and feature request is welcome

Comment author: turpin

I will.

Comment author: turpin

I have tried your new version. It seems less naïve than the previous code, but there are a number of bugs. Here is a patch which corrects 5 of them. The next one is about optional labels:

assert (is_predef_option txt)

fails.

Comment author: @bobzhang

turpin, I did not see your patch and error case

Comment author: turpin

pprintast.patch, I have uploaded it on this page.

Comment author: @bobzhang

you did not update my version revision 13054 in trunk

Comment author: @bobzhang

btw, the new version is written using objects, so you can do hot-patch without touching the source, but bug report is still welcome

Comment author: turpin

The patch I uploaded is against version 13054, unless my svn is lying to me. If you already fixed the bugs ( * ), ( or ), ( -1 ), etc. then you didn't commit the right file.

Currently I'm reverting the 4.00.1 version with the patch I submitted, because it seems to produce syntactically correct code, and I don't care too much about readability. But I will re-test the new version once it's fixed.

Comment author: @bobzhang

your patch is not against my version, and sorry I can not reproduce your case. It would be helpful that you upload your error case.
http://caml.inria.fr/cgi-bin/viewvc.cgi/ocaml/trunk/parsing/pprintast.ml?view=markup&pathrev=13055

Comment author: turpin

I'm sorry, I didn't upload the right patch file. I thougt I did because I had named it pprintast.patch as the old patch that I did fot the 4.00.1 version. I must have forgotten to click the upload button or something. I just uploaded it, as pprintast-13054.patch (I cannot adapt it for 13055 right now but it should mostly work).

Comment author: @bobzhang

would you upload your error cases? add parens everywhere could eliminate the bugs, but it's not the optimal solution, thanks

Comment author: turpin

anything like "assert (f x)" triggers the parenthesis bug. It is clear that in "assert e", "e" must be parenthesized (unless you do a precedence-aware unparsing, which I didn't see in your code).

Comment author: @bobzhang

it is precedence-aware. it passes all the parsetree in trunk even including camlp4. If you have bugs to report, plz file a reproducible case with respect to the trunk version, thx.

Comment author: turpin

I had not seen the methods "simple_expr" ... which implement precedence. The assert bug is fixed in the current svn version (#expression replaced by #simple_expr). As for the other bugs, I didn't check everything, but:

• "( ~+ )" is printed "+"
• "let _ as f = fun x ->..." is printed incorrectly too.

Comment author: @bobzhang

the second is fixed.
The first
let a = (~+) 3 --> let a = 3
this is intended behavior, will it cause a bug in your code?

Comment author: turpin

Yes: I generate code like "let ( ~+ ) = ( ~+ )". So ~+ may be used without argument, in this corner case. Also, as anyone can rebind ~+, it seems dangerous to assume that it means identity.