Implement ring map #9
Conversation
|
I guess it is issue #2 you are trying to solve? The code expected would be the definition of As I wrote on the issue, it would be useful to have a look at the existing |
|
@rand00 i have made a few changes. Due to the limited time i have, kindly bear with the very many follow up questions i will ask |
|
@rand00 I took the time to study the pointers you gave and came up with a potential solution. kindly review and advice Asap. to day being the deadline for application, I would kindly request to to fill out as accepted and if there are any changes, i will keep working till acceptable. |
lib/conntest.ml
Outdated
| @@ -347,7 +347,34 @@ module Make | |||
| .. and ring shouldn't be long | |||
| *) | |||
| Log.debug (fun m -> m "feed_source: packet_index_diff < 0"); | |||
| (*latest*) | |||
| let map_rev_shortcircuit f ring = | |||
There was a problem hiding this comment.
This function should be inside the Ring module
lib/conntest.ml
Outdated
| (*latest*) | ||
| let map_rev_shortcircuit f ring = | ||
| let len = Ring.length ring in | ||
| let new_ring = Ring.copy ring in |
There was a problem hiding this comment.
Try to get the code compiling before committing. When you have all this code which doesn't compile, it also seems suspicious concerning the code looking generated by an AI
lib/conntest.ml
Outdated
| let map_rev_shortcircuit f ring = | ||
| let len = Ring.length ring in | ||
| let new_ring = Ring.copy ring in | ||
| let start = Ring.get_start ring in |
lib/conntest.ml
Outdated
| match Ring.wrap_reverse_i len (i + 1) with | ||
| | None -> ring_acc | ||
| | Some i' -> | ||
| if i' = start || match Ring.get i' new_ring with |
There was a problem hiding this comment.
It is bad style to make such a big expression inside an if
There was a problem hiding this comment.
.. i.e. the code is not very readable
lib/conntest.ml
Outdated
| | Some v -> match f v with | ||
| | None -> true | ||
| | Some v' -> | ||
| let new_ring' = Ring.set i' v' new_ring in |
There was a problem hiding this comment.
There is no Ring.set - Ring.t is intended to be an immutable datastructure
There was a problem hiding this comment.
.. though it is also used as such. It would not be ordinary naming of a function for an immutable datastructure.
lib/conntest.ml
Outdated
| then ring_acc | ||
| else loop i' ring_acc | ||
| in | ||
| let start_i = Option.map_default (fun x -> x - 1) (len - 1) (Ring.wrap_reverse_i len start) in |
There was a problem hiding this comment.
There is no Option.map_default - all this code looks AI generated. Please see mirage/mirage#1402 (comment)
| match idx_opt with | ||
| | None -> i := len | ||
| | Some idx -> | ||
| let value_opt = Ring.get_previous r !i in |
There was a problem hiding this comment.
Ring.get_previous also calls wrap_reverse_i, so there is some duplicate logic here.
| | Some idx -> | ||
| let value_opt = Ring.get_previous r !i in | ||
| match value_opt with | ||
| | None -> i := len |
There was a problem hiding this comment.
This function shouldn't shortcircuit if the Ring contains None on the current position
|
@rand00 kindly ignore my previous commit with the message progress with errors. i tried deleting it without much success. my latest commit compiles successfully |
|
If you intend to continue work on this, then please don't close the PR and start a new one - continue on this PR |
|
@rand00 sorry, already did. should i continue on this one or the other one? i felt like my latest changes were being lost in the middle or the erroneous code i commited three days ago when i was stuck and needed some guidance. |
|
@rand00 can we please proceed in my latest pull request? i have made a few changes already plus i already deleted locally. |
|
@rand00 I have attempted to restore the branch i deleted without success since i deleted both locally and remotely. I do not know how to proceed. |
|
In that case continue with the new PR |
|
@rand00 Thank you. Honestly i was feeling really demoralized. On friday i was so worried about the deadline that I was trying anything that was even remotely close to what was expected just to submit.Then the weekend came and i knew you would not be available to help. The moment i calmed down and had a deeper look at the code itself, it all became so clear. I already pushed new changes. Review them and let me know if anything more needs to be added. The deadline up now, so now is a learning phase |
|
Sounds like you had good mental progress on it (: |
|
@rand00 I did. I went back to the first code i posted where you told me i was a good try. Looked at the recommendations and that helped a lot. This is a learning period, even if i dont get picked for the internship, no regrets. In the end, i have to struggle till I get it right. AI might be useful in essays and what not but when it comes to writing code, a complete joke. |
|
@rand00 if there are any other good issues, I would love to keep contributing. Without the burden of looming deadline, I will revel in the challenge |
@rand00 i have made some progress but still experiencing some errors. monday being the deadline for submission and application kindly guide me on how to solve the errors. i have included comments on what each each part is supposed to do