Simplify processing and remove dead code (error paths) in asmlink#9943
Merged
lthls merged 2 commits intoocaml:trunkfrom Dec 18, 2020
Merged
Simplify processing and remove dead code (error paths) in asmlink#9943lthls merged 2 commits intoocaml:trunkfrom
lthls merged 2 commits intoocaml:trunkfrom
Conversation
nojb
reviewed
Sep 27, 2020
An analysis of the code should convince yourself that the [object_file_name] function is not needed and that its erroring paths are dead code. The reasoning is the following: all the uses of [object_file_name] in asmlink.ml are performed on [obj_name]s which went through [read_file] before. The latter does exactly the same file name lookup and erroring treatement, except not in a stringly manner like [object_file_name] does. This new function derives the same information as [object_file_name] except it does it on the [file] datatype returned by [read_file]. Note that all the erroring code paths of [object_file_name] have been handled by the [read_file] which derived the [file] value. We integrate the logic added by PR ocaml#9011 for empty cmxa here.
This commit does the following four things (it's difficult to them in separate commits that compile). 1) It removes the [read_file] from [scan_file]. Reading the files is done seperately before returning an [obj_infos] list of [file] values. This turns [scan_file] into a function that operates on values of type [file]. 2) In [scan_file] it removes the separate list of [obj_files] introduced by ocaml#9011. We can derive the same list using the function [object_file_name_of_file] introduced in the previous commit on the list of [obj_infos]. Effectively we bring back [scan_file] to the state before ocaml#9011 modulo the [read_file] removal. 3) We derive the list of [obj_files] directly from the [obj_infos] list via [object_file_name_of_file]. Note that the logic introduced by ocaml#9011 is preserved by virtue of [object_file_name_of_file]'s optional result. 4) Deletes [object_file_name] which is no longer used.
b37dbb8 to
d23f49f
Compare
Contributor
Author
|
Gentle ping. |
lthls
approved these changes
Dec 2, 2020
Contributor
lthls
left a comment
There was a problem hiding this comment.
I'm in favour of this. In addition to the cleanup, it removes a (very hypothetical) problem if the lookup of the file by object_file_name returns a different path than the previous lookup by read_file.
As far as I can tell the code is correct.
Contributor
|
@dra27 It seems you assigned yourself to this PR. Do you still plan to look at it or are you fine trusting @dbuenzli and me on this ? |
Contributor
Author
|
@lthls Thanks for the review and approval ! I do not plan to add a changes entry (unless the devs want it of course) but can't add the label myself. |
Contributor
Author
|
Thanks @dra27 for the review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a nop. I need the result of these changes in the context of the library linking RFC. I had a simpler version of it before. However PR #9011 added more complexity in the area and I had to redo it. Since it's a delicate area I would like to avoid having to rethink about all that a third time :-) The result also effectively simplifies #9011.
Basically we separate reading file into
filevalues withread_filefromscan_files(that's what I'm interested in). Thefilevalues allows to derive the info fromscan_filesand theobjfilesto link separately (which #9011 fusioned intoscan_files).The result allows us to get rid of the
object_file_namefunction whose error paths are in fact dead code and which redoes the lookup that was already done byread_filewhich is a bit confusing.The PR should be read commit by commit which have explanations.
I guess @dra27 and/or @xavierleroy would be suitable reviewers.