Skip to main content
Code Review

Return to Answer

added additional suggestion
Source Link
ChrisWue
ChrisWue
  • 20.6k
  • 4
  • 43
  • 107
  1. As J_H mentioned, there are more efficient ways to compute log2 of a u32 - 31 - n.leading_zeros() should do.

    As J_H mentioned, there are more efficient ways to compute log2 of a u32 - 31 - n.leading_zeros() should do.

  2. base.pow() with base = 2 should really be 1 << bits. Are you really intending to use another base value in the future?

    base.pow() with base = 2 should really be 1 << bits. Are you really intending to use another base value in the future?

  3. The two derive macros can be combined in one line and probably also should include Default - #[derive(Copy, Clone, Default)], then you can also write Node::default() instead of Node { expanded: false, filled: false }. Especially in case you add additional properties in the future this makes it easier.

    The two derive macros can be combined in one line and probably also should include Default - #[derive(Copy, Clone, Default)], then you can also write Node::default() instead of Node { expanded: false, filled: false }. Especially in case you add additional properties in the future this makes it easier.

  4. Consider using usize everywhere since your dealing with memory management anyway. Removes the need for a bunch of casts.

    Since usize type size depends on the platform, the improved log2 implementation would then have to be something like std::mem::size_of::<usize>() * 8 - 1 - (n.leading_zeros() as usize)

  1. While a sentinel value to indicate failure is a common programming tool, in Rust functions that can fail should return Result<> so that they can return an actual error and not pick some (more or less arbitrary value) as error indicator. Returning a proper error also let's you include additional details as to what the failure specifically is.

  2. The reason why you currently have unsafe is because you have a global static mutable array. The reason it's unsafe is because it's not thread-safe. Pretty sure that you can do away with it by using a combination of lazy_static (or apparently once_cell in newer compiler versions) and Mutex. The recursive nature of the implementation might be problematic though in this case.

  1. As J_H mentioned, there are more efficient ways to compute log2 of a u32 - 31 - n.leading_zeros() should do.
  2. base.pow() with base = 2 should really be 1 << bits. Are you really intending to use another base value in the future?
  3. The two derive macros can be combined in one line and probably also should include Default - #[derive(Copy, Clone, Default)], then you can also write Node::default() instead of Node { expanded: false, filled: false }. Especially in case you add additional properties in the future this makes it easier.

  1. While a sentinel value to indicate failure is a common programming tool, in Rust functions that can fail should return Result<> so that they can return an actual error and not pick some (more or less arbitrary value) as error indicator. Returning a proper error also let's you include additional details as to what the failure specifically is.

  2. The reason why you currently have unsafe is because you have a global static mutable array. The reason it's unsafe is because it's not thread-safe. Pretty sure that you can do away with it by using a combination of lazy_static (or apparently once_cell in newer compiler versions) and Mutex.

  1. As J_H mentioned, there are more efficient ways to compute log2 of a u32 - 31 - n.leading_zeros() should do.

  2. base.pow() with base = 2 should really be 1 << bits. Are you really intending to use another base value in the future?

  3. The two derive macros can be combined in one line and probably also should include Default - #[derive(Copy, Clone, Default)], then you can also write Node::default() instead of Node { expanded: false, filled: false }. Especially in case you add additional properties in the future this makes it easier.

  4. Consider using usize everywhere since your dealing with memory management anyway. Removes the need for a bunch of casts.

    Since usize type size depends on the platform, the improved log2 implementation would then have to be something like std::mem::size_of::<usize>() * 8 - 1 - (n.leading_zeros() as usize)

  1. While a sentinel value to indicate failure is a common programming tool, in Rust functions that can fail should return Result<> so that they can return an actual error and not pick some (more or less arbitrary value) as error indicator. Returning a proper error also let's you include additional details as to what the failure specifically is.

  2. The reason why you currently have unsafe is because you have a global static mutable array. The reason it's unsafe is because it's not thread-safe. Pretty sure that you can do away with it by using a combination of lazy_static (or apparently once_cell in newer compiler versions) and Mutex. The recursive nature of the implementation might be problematic though in this case.

Source Link
ChrisWue
ChrisWue
  • 20.6k
  • 4
  • 43
  • 107

Small Stuff

  1. As J_H mentioned, there are more efficient ways to compute log2 of a u32 - 31 - n.leading_zeros() should do.
  2. base.pow() with base = 2 should really be 1 << bits. Are you really intending to use another base value in the future?
  3. The two derive macros can be combined in one line and probably also should include Default - #[derive(Copy, Clone, Default)], then you can also write Node::default() instead of Node { expanded: false, filled: false }. Especially in case you add additional properties in the future this makes it easier.

Regarding Professional Code

No I wouldn't accept this as a PR, mainly due to lack of useful documentation that will help the next developer to maintain this even if they don't know what the Buddy algorithm is.

  • Explain what the code does, ideally with a link to Wikipedia or wherever there is a good description of the algorithm (hopefully also one that's likely to be around for a while).
    • Make sure that your code terminology conforms to publicly available explanations - the terms buffer or management size don't really appear anywhere.
    • Also why is the number of nodes you manage dependent on BUFFER_SIZE?
  • Explain how the code implements the algorithm - i.e. what is your design, what are the trade-offs, limitations etc.
  • Document the invariants of the public methods - what do they expect, what will they return under different circumstances and why.

Other problematic things:

  1. Naming - when thinking about memory allocations from a user perspective one thinks about allocate and free - these two terms do not appear anywhere in your code.
  2. A better name for BUFFER_SIZE would be BLOCK_SIZE (to conform to generally available description of the algorithm) and MANAGEMENT_SIZE should be NUM_BLOCKS.

Rustiness

  1. While a sentinel value to indicate failure is a common programming tool, in Rust functions that can fail should return Result<> so that they can return an actual error and not pick some (more or less arbitrary value) as error indicator. Returning a proper error also let's you include additional details as to what the failure specifically is.

  2. The reason why you currently have unsafe is because you have a global static mutable array. The reason it's unsafe is because it's not thread-safe. Pretty sure that you can do away with it by using a combination of lazy_static (or apparently once_cell in newer compiler versions) and Mutex.