Skip to main content
added 49 characters in body
Source Link
Toby Speight
  • 88.7k
  • 14
  • 104
  • 327

Here's something that's not well-defined:

static inline bool is_aligned(const void *ptr, size_t byte_count)
{
    return (uintptr_t) ptr % byte_count == 0;
}

Any kind of object pointer can be converted to uintptr_t, but arithmetic on this integer is unspecified. This test might work on your platform, but there are systems (notably, ia32 segmented memory architectures) where it's likely to be incorrect.


Comparison against nullptr is unnecessary - all pointers convert implicitly to bool.


Arena *const arena = arena_new(nullptr, 100);

The header doesn't say whether passing a null pointer to this function is valid or not. So we can't write any useful tests for this case.


for (size_t i = 0; i < arena->count; ++i) {
    TEST_CHECK(arena->pools[i]->offset == 0);
}

Here, we seem to be relying on the unspecified internals of Arena. That's bad, as we should be able to refactor the implementation without needing to modify the tests - in fact, that's pretty much the definition of refactoring.

Perhaps the Arena needs an accessor we could use to test whether it's empty (or perhaps even to report how much memory it has allocated). We could make this accessor conditionally available (e.g. only if if ARENA_TEST is defined before including its header, or as a separate "arena-test.h" header - or even as a function not mentioned in headers) so as not to make it part of the public interface.

There's more testing of internals here - and it's not at all clear what the magic numbers mean:

uint8_t *const curr_pool = arena->pools[0]->buf;

TEST_CHECK(curr_pool[96] == 0xA5 && curr_pool[97] == 0xA5
    && curr_pool[98] == 0xA5 && curr_pool[99] == 0xA5);

Here's something that's not well-defined:

static inline bool is_aligned(const void *ptr, size_t byte_count)
{
    return (uintptr_t) ptr % byte_count == 0;
}

Any kind of object pointer can be converted to uintptr_t, but arithmetic on this integer is unspecified. This test might work on your platform, but there are systems (notably, ia32 segmented memory architectures) where it's likely to be incorrect.


Comparison against nullptr is unnecessary - all pointers convert implicitly to bool.


Arena *const arena = arena_new(nullptr, 100);

The header doesn't say whether passing a null pointer to this function is valid or not. So we can't write any useful tests for this case.


for (size_t i = 0; i < arena->count; ++i) {
    TEST_CHECK(arena->pools[i]->offset == 0);
}

Here, we seem to be relying on the unspecified internals of Arena. That's bad, as we should be able to refactor the implementation without needing to modify the tests - in fact, that's pretty much the definition of refactoring.

Perhaps the Arena needs an accessor we could use to test whether it's empty (or perhaps even to report how much memory it has allocated). We could make this accessor conditionally available (e.g. only if if ARENA_TEST is defined before including its header, or as a separate "arena-test.h" header) so as not to make it part of the public interface.

There's more testing of internals here - and it's not at all clear what the magic numbers mean:

uint8_t *const curr_pool = arena->pools[0]->buf;

TEST_CHECK(curr_pool[96] == 0xA5 && curr_pool[97] == 0xA5
    && curr_pool[98] == 0xA5 && curr_pool[99] == 0xA5);

Here's something that's not well-defined:

static inline bool is_aligned(const void *ptr, size_t byte_count)
{
    return (uintptr_t) ptr % byte_count == 0;
}

Any kind of object pointer can be converted to uintptr_t, but arithmetic on this integer is unspecified. This test might work on your platform, but there are systems (notably, ia32 segmented memory architectures) where it's likely to be incorrect.


Comparison against nullptr is unnecessary - all pointers convert implicitly to bool.


Arena *const arena = arena_new(nullptr, 100);

The header doesn't say whether passing a null pointer to this function is valid or not. So we can't write any useful tests for this case.


for (size_t i = 0; i < arena->count; ++i) {
    TEST_CHECK(arena->pools[i]->offset == 0);
}

Here, we seem to be relying on the unspecified internals of Arena. That's bad, as we should be able to refactor the implementation without needing to modify the tests - in fact, that's pretty much the definition of refactoring.

Perhaps the Arena needs an accessor we could use to test whether it's empty (or perhaps even to report how much memory it has allocated). We could make this accessor conditionally available (e.g. only if if ARENA_TEST is defined before including its header, or as a separate "arena-test.h" header - or even as a function not mentioned in headers) so as not to make it part of the public interface.

There's more testing of internals here - and it's not at all clear what the magic numbers mean:

uint8_t *const curr_pool = arena->pools[0]->buf;

TEST_CHECK(curr_pool[96] == 0xA5 && curr_pool[97] == 0xA5
    && curr_pool[98] == 0xA5 && curr_pool[99] == 0xA5);
Suggest ways to make the non-public test helper
Source Link
Toby Speight
  • 88.7k
  • 14
  • 104
  • 327

Here's something that's not well-defined:

static inline bool is_aligned(const void *ptr, size_t byte_count)
{
    return (uintptr_t) ptr % byte_count == 0;
}

Any kind of object pointer can be converted to uintptr_t, but arithmetic on this integer is unspecified. This test might work on your platform, but there are systems (notably, ia32 segmented memory architectures) where it's likely to be incorrect.


Comparison against nullptr is unnecessary - all pointers convert implicitly to bool.


Arena *const arena = arena_new(nullptr, 100);

The header doesn't say whether passing a null pointer to this function is valid or not. So we can't write any useful tests for this case.


for (size_t i = 0; i < arena->count; ++i) {
    TEST_CHECK(arena->pools[i]->offset == 0);
}

Here, we seem to be relying on the unspecified internals of Arena. That's bad, as we should be able to refactor the implementation without needing to modify the tests - in fact, that's pretty much the definition of refactoring.

Perhaps the Arena needs an accessor we could use to test whether it's empty (or perhaps even to report how much memory it has allocated). We could make this accessor conditionally available (e.g. only if if ARENA_TEST is defined before including its header, or as a separate "arena-test.h" header) so as not to make it part of the public interface.

There's more testing of internals here - and it's not at all clear what the magic numbers mean:

uint8_t *const curr_pool = arena->pools[0]->buf;

TEST_CHECK(curr_pool[96] == 0xA5 && curr_pool[97] == 0xA5
    && curr_pool[98] == 0xA5 && curr_pool[99] == 0xA5);

Here's something that's not well-defined:

static inline bool is_aligned(const void *ptr, size_t byte_count)
{
    return (uintptr_t) ptr % byte_count == 0;
}

Any kind of object pointer can be converted to uintptr_t, but arithmetic on this integer is unspecified. This test might work on your platform, but there are systems (notably, ia32 segmented memory architectures) where it's likely to be incorrect.


Comparison against nullptr is unnecessary - all pointers convert implicitly to bool.


Arena *const arena = arena_new(nullptr, 100);

The header doesn't say whether passing a null pointer to this function is valid or not. So we can't write any useful tests for this case.


for (size_t i = 0; i < arena->count; ++i) {
    TEST_CHECK(arena->pools[i]->offset == 0);
}

Here, we seem to be relying on the unspecified internals of Arena. That's bad, as we should be able to refactor the implementation without needing to modify the tests - in fact, that's pretty much the definition of refactoring.

Perhaps the Arena needs an accessor we could use to test whether it's empty (or perhaps even to report how much memory it has allocated).

There's more testing of internals here - and it's not at all clear what the magic numbers mean:

uint8_t *const curr_pool = arena->pools[0]->buf;

TEST_CHECK(curr_pool[96] == 0xA5 && curr_pool[97] == 0xA5
    && curr_pool[98] == 0xA5 && curr_pool[99] == 0xA5);

Here's something that's not well-defined:

static inline bool is_aligned(const void *ptr, size_t byte_count)
{
    return (uintptr_t) ptr % byte_count == 0;
}

Any kind of object pointer can be converted to uintptr_t, but arithmetic on this integer is unspecified. This test might work on your platform, but there are systems (notably, ia32 segmented memory architectures) where it's likely to be incorrect.


Comparison against nullptr is unnecessary - all pointers convert implicitly to bool.


Arena *const arena = arena_new(nullptr, 100);

The header doesn't say whether passing a null pointer to this function is valid or not. So we can't write any useful tests for this case.


for (size_t i = 0; i < arena->count; ++i) {
    TEST_CHECK(arena->pools[i]->offset == 0);
}

Here, we seem to be relying on the unspecified internals of Arena. That's bad, as we should be able to refactor the implementation without needing to modify the tests - in fact, that's pretty much the definition of refactoring.

Perhaps the Arena needs an accessor we could use to test whether it's empty (or perhaps even to report how much memory it has allocated). We could make this accessor conditionally available (e.g. only if if ARENA_TEST is defined before including its header, or as a separate "arena-test.h" header) so as not to make it part of the public interface.

There's more testing of internals here - and it's not at all clear what the magic numbers mean:

uint8_t *const curr_pool = arena->pools[0]->buf;

TEST_CHECK(curr_pool[96] == 0xA5 && curr_pool[97] == 0xA5
    && curr_pool[98] == 0xA5 && curr_pool[99] == 0xA5);
Source Link
Toby Speight
  • 88.7k
  • 14
  • 104
  • 327

Here's something that's not well-defined:

static inline bool is_aligned(const void *ptr, size_t byte_count)
{
    return (uintptr_t) ptr % byte_count == 0;
}

Any kind of object pointer can be converted to uintptr_t, but arithmetic on this integer is unspecified. This test might work on your platform, but there are systems (notably, ia32 segmented memory architectures) where it's likely to be incorrect.


Comparison against nullptr is unnecessary - all pointers convert implicitly to bool.


Arena *const arena = arena_new(nullptr, 100);

The header doesn't say whether passing a null pointer to this function is valid or not. So we can't write any useful tests for this case.


for (size_t i = 0; i < arena->count; ++i) {
    TEST_CHECK(arena->pools[i]->offset == 0);
}

Here, we seem to be relying on the unspecified internals of Arena. That's bad, as we should be able to refactor the implementation without needing to modify the tests - in fact, that's pretty much the definition of refactoring.

Perhaps the Arena needs an accessor we could use to test whether it's empty (or perhaps even to report how much memory it has allocated).

There's more testing of internals here - and it's not at all clear what the magic numbers mean:

uint8_t *const curr_pool = arena->pools[0]->buf;

TEST_CHECK(curr_pool[96] == 0xA5 && curr_pool[97] == 0xA5
    && curr_pool[98] == 0xA5 && curr_pool[99] == 0xA5);