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
two minor bugs in pprintast: (*) -> ( * ), {... _} -> {...; _} #5801
Comments
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). |
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. |
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:
|
Comment author: @bobzhang the second is fixed. |
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. |
Original bug ID: 5801
Reporter: turpin
Assigned to: @bobzhang
Status: closed (set by @xavierleroy on 2015-12-11T18:08:19Z)
Resolution: not a bug
Priority: low
Severity: trivial
Version: 4.00.1
Category: misc
Bug description
The attached patch corrects the above two syntax errors, and treats "as" in the same way as variables (imagine "match ... with _ as ( * ) -> ...")
File attachments
The text was updated successfully, but these errors were encountered: