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
Unclear code in Camlp4/Struct/Lexer.mll #5564
Comments
Comment author: @diml Actually we need to read only one token at a time for the toplevel since lexing a token may call Toploop.read_interactive_input. But maybe it is safe to read multiple tokens at at time only for files. I will look closer at this. BTW, does the proposed change fix your performance issues ? |
Comment author: Drakken This issue doesn't really affect me. |
Comment author: @gasche Drakken, I appreciate that people participate on the bug tracker, and I do think that there is something slightly odd with this code (the best way to know would probably to ask the original author) that might be the sign of something amiss, or would at least merit a cleanup or an explanatory comment. However, this is really not a bug. When reporting, please select the appropriate severity: "minor/major/block" is reserved to issues that are acutally bugs. "Feature" is for feature requests (that should be used with care) and "text", "tweak" and "trivial" are more appropriate for the rest ("text" for documentation issues, and tweak/trivial for the rest). |
Comment author: Drakken Okay, I'll use "tweak" if I ever see something similar. Maybe it would help to have a category called something like "factoring" or "code quality". I knew the code was working, but it looks like a mistake where the dev intended to read more chars. I would say the degree of obfuscation is significant, and "tweak" and "trivial" don't really suggest that. let lexing_store s buff max = |
Comment author: @gasche I'm not convinced the "code quality" reports on the bug tracker are such a good idea. As I said, OCaml developpers tend to be extremely busy (nobody works full-time on OCaml development), and want to concentrate their development time on important matter (such as fixing real bugs and getting a release out). Discussing small code readability issues on the bug tracker is simply too costly in the current model, because it will force those devs to be involved when they really should work on other things. So I think that if you want to go on reading code and commenting on awkward places (which is, in itself, a good think), there should be another process that would not involve the bug tracker directly. For example, you could aggregate and publish your findings on a web page somewhere and contact a relevant developer¹ directly, so that they can be processed in batch modes and without sending tons of emails to most OCaml developpers. ¹: Who is "a relevant developer"? I'm not sure whether Jérémie, the current Camlp4 maintainer, is interested in such issues, but I suspect he might be. Otherwise you could contact me directly (gabriel dot scherer at gmail), but this would be rather low priority. Besides, Camlp4 is a delicate and complex piece of software. Changes in this area have a tendency to break existing user extensions, and that is really not a good thing. If you want to suggest changes, you should test them extremely thoroughly (at least by going through Camlp4 bootstrap process; warning, this is not a small task). And ultimately most proposed changes would probably fall under the pragmatic rule of "if it ain't broke, don't fix it" -- though explanatory comments are not subject to this rule as they don't change the semantics at all. Finally, if you are doing this with the laudable goal to familiarize yourself with the code, I must say there are other things that would be more immediately useful and on which you could help. Some of the bugs marked "acknowledged", "confirmed" or "resolved > suspended" could benefit from a patch proposal -- but some areas are more sensible than other. See for example On a strictly personal note, I believe that there are things to improve with Ocamlbuild that could benefit from external contributions. See the list of ocamlbuild bugs, most things could benefit from a proposed patch (including, I believe, those assigned to xclerc): http://caml.inria.fr/mantis/view_all_bug_page.php If you're interested in Camlp4 specifically, I'm not overly confident in code changes, but there are certainly things to improve on the documentation side -- on the wiki: |
Comment author: @damiendoligez I have one remark to add to Gabriel's: if you want a change like that accepted, you have to demonstrate its utility: take your "Maybe process a large source file and check for performance problems.", remove the Maybe, and give us some performance numbers. |
Comment author: @diml Camlp4 is now a separate project. Please re-open a ticket on github [1] if you are still interested in seeing the original issue fixed. |
Original bug ID: 5564
Reporter: Drakken
Assigned to: @diml
Status: closed (set by @diml on 2016-12-07T17:27:34Z)
Resolution: suspended
Priority: low
Severity: tweak
Target version: later
Category: -for Camlp4 use https://github.com/ocaml/camlp4/issues
Tags: patch
Monitored by: @gasche @diml
Bug description
In camlp4/Camlp4/Struct/Lexer.mll,
line 458 ("succ n") reads only one character
from the stream instead of recursing.
450 let lexing_store s buff max =
451 let rec self n s =
452 if n >= max then n
453 else
454 match Stream.peek s with
455 | Some x ->
456 Stream.junk s;
457 buff.[n] <- x;
458 succ n
459 | _ -> n
460 in
461 self 0 s
Steps to reproduce
Maybe process a large source file and
check for performance problems.
Additional information
Proposed changes:
451:
< let rec self n s =
The text was updated successfully, but these errors were encountered: