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

WIP: Replace all memory allocations with fallible allocations and swap the… #29

Merged
merged 23 commits into from
Feb 7, 2020

Conversation

ahomescu
Copy link
Contributor

@ahomescu ahomescu commented Feb 4, 2020

… global allocator in runtests.rs with the tests' own

… global allocator in runtests.rs with the tests' own
@ahomescu ahomescu self-assigned this Feb 4, 2020
@ahomescu ahomescu changed the title Replace all memory allocations with fallible allocations and swap the… WIP: Replace all memory allocations with fallible allocations and swap the… Feb 4, 2020
src/lib/xmlparse.rs Outdated Show resolved Hide resolved
@@ -13,6 +13,8 @@
#![feature(main)]
#![feature(const_in_array_repeat_expressions)]
#![feature(ptr_wrapping_offset_from)]
#![feature(try_reserve)]
#![feature(alloc_layout_extra)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to not add nightly features?

If we do add them, be sure to update #18

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading rust-lang/rust#55724 (comment) makes me think they'll stabilize more of Layout soon. There's also been some recent discussion on stabilizing try_reserve.

Copy link
Contributor Author

@ahomescu ahomescu Feb 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One alternative Stephen and I were discussing earlier is to have a nightly or fallible feature that conditionally enables fallible allocation, and fall back to panics if building for stable Rust. We really, really need the nightly features for runtests, but could allow panics outside of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized something else: alloc_layout_extra is temporary, since once we replace all pointers with slices/boxes/etc we won't need to manually allocate any memory, so that feature will go away. We'll still need try_reserve, but we can enable that conditionally.

Copy link
Contributor

@TheDan64 TheDan64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple small comments

src/lib/xmlparse.rs Show resolved Hide resolved
if let XML_ParserStruct {ref m_mem, ref mut m_tempPool, ref mut m_temp2Pool, ..} = &mut *parser {
m_tempPool.init(m_mem);
m_temp2Pool.init(m_mem);
if let XML_ParserStruct {ref mut m_tempPool, ref mut m_temp2Pool, ..} = &mut *parser {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to remove this if let binding now. I added it so that the m_tempPool field could be used mutably along with m_mem being used immutably in the same line

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, to avoid a compiler warning about the irrefutable if-let pattern, that let binding could have been:

{
  let XML_ParserStruct {ref mut m_tempPool, ref mut m_temp2Pool, ..} = &mut *parser;
  // do stuff..
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That whole conditional is gone now, but the point about the let binding is good to know.

Copy link
Contributor

@rinon rinon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments inline, but this LGTM.

src/lib/xmlparse.rs Outdated Show resolved Hide resolved
src/lib/xmlparse.rs Outdated Show resolved Hide resolved
src/lib/xmlparse.rs Outdated Show resolved Hide resolved
.expect("non-null function pointer")($ptr)
($ptr:expr $(,)?) => {
// FIXME: get the actual layout somehow
alloc::dealloc($ptr as *mut u8, Layout::new::<u8>())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if someone swaps the global allocator on us between allocation and freeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad things happen. I don't have a good way of handling that case, and I don't think Rust does either. The global allocator is supposed to be a singleton.

@ahomescu ahomescu merged commit 0361805 into master Feb 7, 2020
@ahomescu ahomescu deleted the swap_runtests_alloc branch May 1, 2020 00:08
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.

3 participants