-
Notifications
You must be signed in to change notification settings - Fork 283
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
xapi-idl: Delete String.{explode,implode} functions #5881
Conversation
15eb79b
to
7c985e8
Compare
I've run a suite to verify that cookie parsing is not disturbed. Most of the code is unit tested (split_f and the device numbers in particular). I think this is ready to go |
@@ -81,24 +67,28 @@ module String = struct | |||
in | |||
concat "" (fold_right aux string []) | |||
|
|||
(** Take a predicate and a string, return a list of strings separated by | |||
runs of characters where the predicate was true (excluding those characters from the result) *) | |||
let split_f p str = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather avoid importing Astring if not needed, the function is well tested with unit tests
) | ||
else | ||
Normal | ||
match List.assoc_opt "class" params with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not related to String.implode, might've been useful to have them in a separate commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update the commit message with a description of what else to expect in this commit? (e.g. the assoc_opt optimization, and I also see some device number handling refactoring)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've described both changes in the commit. Let me know if it's clarifying enough
e0dae0f
to
6ec4ef6
Compare
5694ec7
to
50c6f36
Compare
if not (String.Sub.is_empty x) then | ||
None | ||
else | ||
Some (pre, post) | ||
in | ||
(* Parse a string "123p456" into x, y where x = 123 and y = 456 *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use sscanf
with %dp%d
? A lot of this parsing code looks too complicated.
let f s = Scanf.sscanf s "%dp%d" (fun x y -> (x,y));;
utop # f "123p456";;
- : int * int = (123, 456)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This module has a lot of code that looks too complicated and this is a step in the right direction - especially for functions that are used frequently. I suspect more simplification is possible when it comes to parsing and decomposing strings.
These are highly inefficient. Also changes some functions to be able to have less types and make normal usage clearer. This comes at the cost of having to destructure the main type when pattern-matching it. Moves the device_number tests to its own executable to easily iterate on the tests. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
Use the standard ones instead Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
The function is implemented using foldleft using a weird form, use a recursive form instead. Unfortunately the function was introduced in OCaml 5.1, so it had to be moved to Listext so it can be reused. Signed-off-by: Pau Ruiz Safont <pau.ruizsafont@cloud.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Original code looks quite complex, but as long as there are good test coverage this should be good
These are highly inefficient.
Also changes some functions to be able to have less types and make normal usage
clearer. This comes at the cost of having to destructure the main type when
pattern-matching it.
Moves the device_number tests to its own executable to easily iterate on the
tests.
Drafting as it needs to pass tests regarding urlencoding