Skip to main content
deleted 3 characters in body
Source Link

This looks pretty good overall.

All I am really missing is tests. Some methods and functions may be covered by unit tests.

Some further, rather opinionated, nitpicks:

  • I don't like the ret variables in mmap and munmap. It's a bad name and they're only ever used once.
  • I also don't like the uncommented comparison to -1 in munmap. It's a magic number and may warrant a constant or a comment like // libc::munmap() failed. A good counter example of what I'd expect is your ret == libc::MAP_FAILED which is self-documenting.
  • Protection::None looks like you could remove it from the enum and use Option<Protection> on first glance. However, this would make implementing the BitOr stuff a nightmare, though I don't see thoseit being used anywhere anyway.

This looks pretty good overall.

All I am really missing is tests. Some methods and functions may be covered by unit tests.

Some further, rather opinionated, nitpicks:

  • I don't like the ret variables in mmap and munmap. It's a bad name and they're only ever used once.
  • I also don't like the uncommented comparison to -1 in munmap. It's a magic number and may warrant a constant or a comment like // libc::munmap() failed. A good counter example of what I'd expect is your ret == libc::MAP_FAILED which is self-documenting.
  • Protection::None looks like you could remove it from the enum and use Option<Protection> on first glance. However, this would make implementing the BitOr stuff a nightmare, though I don't see those being used anywhere anyway.

This looks pretty good overall.

All I am really missing is tests. Some methods and functions may be covered by unit tests.

Some further, rather opinionated, nitpicks:

  • I don't like the ret variables in mmap and munmap. It's a bad name and they're only ever used once.
  • I also don't like the uncommented comparison to -1 in munmap. It's a magic number and may warrant a constant or a comment like // libc::munmap() failed. A good counter example of what I'd expect is your ret == libc::MAP_FAILED which is self-documenting.
  • Protection::None looks like you could remove it from the enum and use Option<Protection> on first glance. However, this would make implementing the BitOr stuff a nightmare, though I don't see it being used anywhere anyway.
Source Link

This looks pretty good overall.

All I am really missing is tests. Some methods and functions may be covered by unit tests.

Some further, rather opinionated, nitpicks:

  • I don't like the ret variables in mmap and munmap. It's a bad name and they're only ever used once.
  • I also don't like the uncommented comparison to -1 in munmap. It's a magic number and may warrant a constant or a comment like // libc::munmap() failed. A good counter example of what I'd expect is your ret == libc::MAP_FAILED which is self-documenting.
  • Protection::None looks like you could remove it from the enum and use Option<Protection> on first glance. However, this would make implementing the BitOr stuff a nightmare, though I don't see those being used anywhere anyway.