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

Editing do_alloc for reducing LLVM IR #341

Merged
merged 1 commit into from Jun 9, 2022

Conversation

JustForFun88
Copy link
Contributor

  1. I think this will speed up compilation, since one way or another everything will come down to this code (but I didn’t compare performance).

  2. I don’t know if the compiler was able to optimize that old code, but it literally checked the same thing twice (due to map and map_err in a row, and roughly speaking it came down to:

    pub fn do_alloc<A: Allocator>(alloc: &A, layout: Layout) -> Result<NonNull<u8>, ()> {
        match match alloc.allocate(layout) {
            Ok(ptr) => Ok(ptr.as_non_null_ptr()),
            Err(e) => Err(e),
        } {
            Ok(ptr) => Ok(ptr),
            Err(_) => Err(()),
        }
    }

    And when the code is written explicitly, it is immediately clear that it looks strange. And why force the compiler to think beyond the need?

  3. And finally, in my opinion, the readability of the code has not only not decreased, but even increased.

@Amanieu
Copy link
Member

Amanieu commented Jun 9, 2022

@bors r+

1 similar comment
@Amanieu
Copy link
Member

Amanieu commented Jun 9, 2022

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 9, 2022

📌 Commit 9d23b42 has been approved by Amanieu

@bors
Copy link
Collaborator

bors commented Jun 9, 2022

⌛ Testing commit 9d23b42 with merge 67e3f87...

@bors
Copy link
Collaborator

bors commented Jun 9, 2022

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 67e3f87 to master...

@bors bors merged commit 67e3f87 into rust-lang:master Jun 9, 2022
@JustForFun88 JustForFun88 deleted the edit_do_alloc branch June 9, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants