Refactor the main cfg_if in backends.rs#808
Closed
bushrat011899 wants to merge 2 commits intorust-random:masterfrom
Closed
Refactor the main cfg_if in backends.rs#808bushrat011899 wants to merge 2 commits intorust-random:masterfrom
cfg_if in backends.rs#808bushrat011899 wants to merge 2 commits intorust-random:masterfrom
Conversation
Refactored to ensure the main cfg_if consists of getrandom_backends, then selection exclusively by target_os. This means ordering of branches is far less consequential. Instead, consequential ordering is nested within the arm that selects a particular OS.
Contributor
Author
|
I'm not really sure why Test / iOS Simulator failed, I don't believe it's because of this PR, but I think I'd need some assistance if this PR did somehow cause the issue. |
Member
|
The current state is fine and I don't think it needs LLM-based refactorings. |
Contributor
Author
|
Understood, just clarifying I don't use LLMs or any other AI tooling. |
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
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.
Objective
Within backends.rs, the selection of a backend is controlled by a single monolithic
cfg_ifstatement. In my opinion, it's quite confronting to work on in its current form, as the ordering of branches is highly consequential and effectively spans the full ~200 lines of the file. Ideally, this statement would be easier to reason about.Observations
With a bit of squinting, it's possible to perceive the
cfg_ifas having 3 main "chunks" (in order):getrandom_backendcompile_errorcatch-all to inform the user when a backend isn't availableSection 1's ordering within itself, in my opinion, does not matter. Since these flags are set by the user, it is very difficult to inadvertently have two
getrandom_backend's selected at the same time. Even if such a case occurred, I don't believegetrandomcan reasonably infer what the user meant anyway. Section 3 is also a singleelsebranch, so its ordering is trivially inconsequential.Section 2 is where ordering currently matters. For example, under
target_os = "linux", we select (in order):linux_rawiftarget_env = ""linux_android_with_fallbackif the Linux kernel version may be older than 3.17getrandomotherwiseBut branches 2 and 3 are intermingled with other configurations which may use those backends, spreading the Linux backend selection logic over many lines of code (made worse by the complexity of the configuration statement detecting legacy versions of the Linux kernel).
I believe this arrangement is specifically chosen for a combination of evolution (backends added over time, slowly increasing the complexity of the logic), and a desire to reduce the number of locations where a
mod backend;statement is used.Solution
target_os, allowing its ordering to be inconsequential.target_oscould be selected (e.g.,linux), moved the ordered selection into an innercfg_ifstatement. This matches howwasibackends were selected based ontarget_env, so there is precedent for this style.horizon), included a specificcompile_errorstatement to better inform the user of what went wrong and what they can do to resolve the issue.cfg(windows)withcfg(target_os = "windows"). They are equivalent, buttarget_osbetter matches the other branches, so I believe it is easier to understand.compile_errorfortarget_os = "uefi"that calls out the opt-inefi_rngbackend.target_arch = wasm32clause from the WASI branches as it is redundant.target_os = "none"andtarget_os = "unknown". This covers cases where there is either no OS, or the OS is unknown. In this case, an innercfg_ifis added to switch on thetarget_arch. Baremetal x86(64) and Aarch64 is supported throughrdrandandrndrrespectively, andwasm32is supported if and only ifwasm_jsis enabled. Ifwasm_jsisn't enabled, the existing compiler error for that situation is thrown. If the target architecture isn't one of those 4 then a specificcompile_erroris thrown.Notes
getrandom, so existing users are already defining agetrandom_backendand will be unaffected).cfg_if.crossfig, which I've designed to try and make managing complex configurations easier. I chosegetrandomto play with as it contains one of the most complexcfg_ifstatements I'm aware of.