Skip to main content
added 124 characters in body
Source Link
chux
  • 35.7k
  • 2
  • 41
  • 95

Just a little review.

... code style, variable names and API may change.
(OP)

Array indexing

Code has typedef unsigned int index_t;, which I take as later code may use:

typedef size_t             index_t; // or
typedef unsigned long      index_t; // or
typedef uint32_t           index_t; // or
typedef unsigned char      index_t; // or
typedef some_unsigned_type index_t; // or
...

Given usage as an array index, size_t makes the most sense as it is the Goldilocks type for general array indexing, not too narrow nor too wide.


Consider maintenance

Yet look how code may break if that type definition is changed:

// What if we change this line?
// typedef unsigned int index_t;
typedef some_updated_unsigned_type index_t;

// This line needs updating
#define INDEX_END UINT_MAX

// So does this
#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))

// Maybe this as ~ORIGIN_MASK is extra wide if `index_t` was narrower than `unsigned`.
#define INDEX_MASK (~ORIGIN_MASK)

// And this
if (index >= msgq->n) {

// More ??

Sometimes, that is life, a type changed involves re-assessing many lines of code.

Yet how to reduce the impact?

INDEX_END (which I think should be called INDEX_MAX) can use ((index_t)-1) as that computation yields the correct value as the unsigned type definition of index_t changes.

//#define INDEX_END UINT_MAX
#define INDEX_END ((index_t)-1)

CONSUMED_FLAG is a little tricky:

//#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))
#define CONSUMED_FLAG (INDEX_END - INDEX_END/2)

... or #define CONSUMED_FLAG ((index_t)1 << (IMAX_BITS(INDEX_END) - 1))

Use a consistent type:

typedef struct msgq {
    // unsigned n;
    index_t n;  // <-----------
    ...

    if (index >= msgq->n) {
        return NULL;
    }

Some code checkers like MISRA may whine about casting, so we could use other techniques.

Or add comments that describe how index_t impacts INDEX_END and CONSUMED_FLAG.

Just a little review.

... code style, variable names and API may change.
(OP)

Array indexing

Code has typedef unsigned int index_t;, which I take as later code may use:

typedef size_t             index_t; // or
typedef unsigned long      index_t; // or
typedef uint32_t           index_t; // or
typedef unsigned char      index_t; // or
typedef some_unsigned_type index_t; // or
...

Given usage as an array index, size_t makes the most sense as it is the Goldilocks type for general array indexing, not too narrow nor too wide.


Consider maintenance

Yet look how code may break if that type definition is changed:

// What if we change this line?
// typedef unsigned int index_t;
typedef some_updated_unsigned_type index_t;

// This line needs updating
#define INDEX_END UINT_MAX

// So does this
#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))

// Maybe this as ~ORIGIN_MASK is extra wide if `index_t` was narrower than `unsigned`.
#define INDEX_MASK (~ORIGIN_MASK)

// And this
if (index >= msgq->n) {

// More ??

Sometimes, that is life, a type changed involves re-assessing many lines of code.

Yet how to reduce the impact?

INDEX_END (which I think should be called INDEX_MAX) can use ((index_t)-1) as that computation yields the correct value as the unsigned type definition of index_t changes.

//#define INDEX_END UINT_MAX
#define INDEX_END ((index_t)-1)

CONSUMED_FLAG is a little tricky:

//#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))
#define CONSUMED_FLAG (INDEX_END - INDEX_END/2)

Use a consistent type:

typedef struct msgq {
    // unsigned n;
    index_t n;  // <-----------
    ...

    if (index >= msgq->n) {
        return NULL;
    }

Some code checkers like MISRA may whine about casting, so we could use other techniques.

Or add comments that describe how index_t impacts INDEX_END and CONSUMED_FLAG.

Just a little review.

... code style, variable names and API may change.
(OP)

Array indexing

Code has typedef unsigned int index_t;, which I take as later code may use:

typedef size_t             index_t; // or
typedef unsigned long      index_t; // or
typedef uint32_t           index_t; // or
typedef unsigned char      index_t; // or
typedef some_unsigned_type index_t; // or
...

Given usage as an array index, size_t makes the most sense as it is the Goldilocks type for general array indexing, not too narrow nor too wide.


Consider maintenance

Yet look how code may break if that type definition is changed:

// What if we change this line?
// typedef unsigned int index_t;
typedef some_updated_unsigned_type index_t;

// This line needs updating
#define INDEX_END UINT_MAX

// So does this
#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))

// Maybe this as ~ORIGIN_MASK is extra wide if `index_t` was narrower than `unsigned`.
#define INDEX_MASK (~ORIGIN_MASK)

// And this
if (index >= msgq->n) {

// More ??

Sometimes, that is life, a type changed involves re-assessing many lines of code.

Yet how to reduce the impact?

INDEX_END (which I think should be called INDEX_MAX) can use ((index_t)-1) as that computation yields the correct value as the unsigned type definition of index_t changes.

//#define INDEX_END UINT_MAX
#define INDEX_END ((index_t)-1)

CONSUMED_FLAG is a little tricky:

//#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))
#define CONSUMED_FLAG (INDEX_END - INDEX_END/2)

... or #define CONSUMED_FLAG ((index_t)1 << (IMAX_BITS(INDEX_END) - 1))

Use a consistent type:

typedef struct msgq {
    // unsigned n;
    index_t n;  // <-----------
    ...

    if (index >= msgq->n) {
        return NULL;
    }

Some code checkers like MISRA may whine about casting, so we could use other techniques.

Or add comments that describe how index_t impacts INDEX_END and CONSUMED_FLAG.

Source Link
chux
  • 35.7k
  • 2
  • 41
  • 95

Just a little review.

... code style, variable names and API may change.
(OP)

Array indexing

Code has typedef unsigned int index_t;, which I take as later code may use:

typedef size_t             index_t; // or
typedef unsigned long      index_t; // or
typedef uint32_t           index_t; // or
typedef unsigned char      index_t; // or
typedef some_unsigned_type index_t; // or
...

Given usage as an array index, size_t makes the most sense as it is the Goldilocks type for general array indexing, not too narrow nor too wide.


Consider maintenance

Yet look how code may break if that type definition is changed:

// What if we change this line?
// typedef unsigned int index_t;
typedef some_updated_unsigned_type index_t;

// This line needs updating
#define INDEX_END UINT_MAX

// So does this
#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))

// Maybe this as ~ORIGIN_MASK is extra wide if `index_t` was narrower than `unsigned`.
#define INDEX_MASK (~ORIGIN_MASK)

// And this
if (index >= msgq->n) {

// More ??

Sometimes, that is life, a type changed involves re-assessing many lines of code.

Yet how to reduce the impact?

INDEX_END (which I think should be called INDEX_MAX) can use ((index_t)-1) as that computation yields the correct value as the unsigned type definition of index_t changes.

//#define INDEX_END UINT_MAX
#define INDEX_END ((index_t)-1)

CONSUMED_FLAG is a little tricky:

//#define CONSUMED_FLAG ((index_t)1 << (UINT_WIDTH - 1))
#define CONSUMED_FLAG (INDEX_END - INDEX_END/2)

Use a consistent type:

typedef struct msgq {
    // unsigned n;
    index_t n;  // <-----------
    ...

    if (index >= msgq->n) {
        return NULL;
    }

Some code checkers like MISRA may whine about casting, so we could use other techniques.

Or add comments that describe how index_t impacts INDEX_END and CONSUMED_FLAG.