Skip to content
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

Closed
vicuna opened this issue Mar 28, 2012 · 7 comments
Closed

Unclear code in Camlp4/Struct/Lexer.mll #5564

vicuna opened this issue Mar 28, 2012 · 7 comments

Comments

@vicuna
Copy link

vicuna commented Mar 28, 2012

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 =

let rec self n =
458:
< succ n
self (succ n)
461:
< self 0 s
self 0

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

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 ?

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

Comment author: Drakken

This issue doesn't really affect me.
It just seems like a very obfuscated
way to read a single character.

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

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).

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

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.
Here's the code to read one character:

let lexing_store s buff max =
assert (max > 0);
match Stream.peek s with
| Some x ->
Stream.junk s;
buff.[0] <- x;
1
| None -> 0

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

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

#3352

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:

http://brion.inria.fr/gallium/index.php/Camlp4

@vicuna
Copy link
Author

vicuna commented Sep 21, 2012

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.
Otherwise, it's definitely "if it ain't broke, don't fix it", or at best "let's simplify the code and just read one character".

@vicuna
Copy link
Author

vicuna commented Dec 7, 2016

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.

[1] https://github.com/ocaml/camlp4/issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant