patterncMinor
Managing a bit array
Viewed 0 times
arraybitmanaging
Problem
I'm training a bit with C, and I'm trying to build a bit array. I would like to have some comments about my code, because I'm not sure that it's the best way to do it.
```
// BAL.c
#include
#include
#include
#include
#include "BAL.h"
#define BAL_Mask(n) (1 nBits; ++i)
p->pBits[i] &= q->pBits[i];
}
_Bool
BAL_Any(BAL_Array *p)
{
assert(p != NULL);
for (size_t i = 0; i nBits; ++i) {
if (BAL_Get(p, i) != 0)
return 1;
}
return 0;
}
_Bool
BAL_Compar(BAL_Array p, BAL_Array q)
{
assert(p != NULL);
assert(q != NULL);
if (p->nSize != q->nSize)
return 0;
for (size_t i = p->nBits; i > 0; --i) {
if (BAL_Get(p, i) != BAL_Get(q, i))
return 0;
}
return 1;
}
size_t
BAL_Count(BAL_Array *p)
{
assert(p != NULL);
size_t n = 0;
for (size_t i = 0; i nBits; ++i) {
if (BAL_Get(p, i) != 0)
++n;
}
return n;
}
void
BAL_End(BAL_Array *p)
{
assert(p != NULL);
free(p->pBits);
free(p);
}
inline unsigned char
BAL_Get(BAL_Array *p, size_t i)
{
assert(p != NULL);
assert(i nBits);
return p->pBits[BAL_Convert(i)] & BAL_Mask(i);
}
inline size_t
BAL_GetSize(BAL_Array *p)
{
assert(p != NULL);
return p->nBits;
}
BAL_Array *
BAL_Init(size_t n)
{
assert(n > 0);
BAL_Array p = malloc(sizeof p);
assert(p != NULL);
p->nSize = BAL_Convert(n + CHAR_BIT - 1);
p->pBits = calloc(p->nSize, 1);
p->nBits = n;
return p;
}
BAL_Array *
BAL_Load(const char *s)
{
assert(s != NULL);
size_t size = strlen(s);
BAL_Array *pRet = BAL_Init(size);
for (size_t i = 0; i nBits; ++i) {
p->pBits[i] >>= m;
p->pBits[i] |= BAL_Get(p, i + 1) ? 1 : 0;
}
}
void
BAL_Not(BAL_Array *p)
{
assert(p != NULL);
for (size_t i = 0; i nBits; ++i)
p->pBits[i] = ~p->pBits[i];
}
void
BAL_Or(BAL_Array p, BAL_Array q)
{
assert(p != NULL);
assert(q != NULL);
```
// BAL.c
#include
#include
#include
#include
#include "BAL.h"
#define BAL_Mask(n) (1 nBits; ++i)
p->pBits[i] &= q->pBits[i];
}
_Bool
BAL_Any(BAL_Array *p)
{
assert(p != NULL);
for (size_t i = 0; i nBits; ++i) {
if (BAL_Get(p, i) != 0)
return 1;
}
return 0;
}
_Bool
BAL_Compar(BAL_Array p, BAL_Array q)
{
assert(p != NULL);
assert(q != NULL);
if (p->nSize != q->nSize)
return 0;
for (size_t i = p->nBits; i > 0; --i) {
if (BAL_Get(p, i) != BAL_Get(q, i))
return 0;
}
return 1;
}
size_t
BAL_Count(BAL_Array *p)
{
assert(p != NULL);
size_t n = 0;
for (size_t i = 0; i nBits; ++i) {
if (BAL_Get(p, i) != 0)
++n;
}
return n;
}
void
BAL_End(BAL_Array *p)
{
assert(p != NULL);
free(p->pBits);
free(p);
}
inline unsigned char
BAL_Get(BAL_Array *p, size_t i)
{
assert(p != NULL);
assert(i nBits);
return p->pBits[BAL_Convert(i)] & BAL_Mask(i);
}
inline size_t
BAL_GetSize(BAL_Array *p)
{
assert(p != NULL);
return p->nBits;
}
BAL_Array *
BAL_Init(size_t n)
{
assert(n > 0);
BAL_Array p = malloc(sizeof p);
assert(p != NULL);
p->nSize = BAL_Convert(n + CHAR_BIT - 1);
p->pBits = calloc(p->nSize, 1);
p->nBits = n;
return p;
}
BAL_Array *
BAL_Load(const char *s)
{
assert(s != NULL);
size_t size = strlen(s);
BAL_Array *pRet = BAL_Init(size);
for (size_t i = 0; i nBits; ++i) {
p->pBits[i] >>= m;
p->pBits[i] |= BAL_Get(p, i + 1) ? 1 : 0;
}
}
void
BAL_Not(BAL_Array *p)
{
assert(p != NULL);
for (size_t i = 0; i nBits; ++i)
p->pBits[i] = ~p->pBits[i];
}
void
BAL_Or(BAL_Array p, BAL_Array q)
{
assert(p != NULL);
assert(q != NULL);
Solution
Quite clean implementation, but some remarks:
-
The header shouldn't compile. It references
-
BAL_Any can be faster if you just compare the characters in the pBits array to zero, you don't need to check each individual bit. Same with BAL_Compar.
-
Any particular reason why you denied BAL_Compar that last 'e'?
-
-
Do you want BAL_Load to make "1000" from, say, "1337" and "1ABC"?
-
As Jeff Mercado already said, asserts aren't the best way for error handling, especially for errors the caller didn't cause. Checking for null pointers is ok, but your BAL_Init and BAL_Load should return NULL if an error occured. That way, the caller can at least try to recover.
-
I'm missing a BAL_Toggle.
-
You should consider moving the struct definition into the implementation file. You only use pointers to it, so you can just as well make it opaque. That way, you can change the implementation (i.e. struct-layout) of the array without having to recompile everything that included the header.
-
The header shouldn't compile. It references
size_t, but doesn't include stdlib.h.-
BAL_Any can be faster if you just compare the characters in the pBits array to zero, you don't need to check each individual bit. Same with BAL_Compar.
-
Any particular reason why you denied BAL_Compar that last 'e'?
-
strlen is unsafe. Consider adding a size argument to BAL_Load so the user is responsible for passing the right size. This also enables the use of BAL_Load with substrings.-
Do you want BAL_Load to make "1000" from, say, "1337" and "1ABC"?
-
As Jeff Mercado already said, asserts aren't the best way for error handling, especially for errors the caller didn't cause. Checking for null pointers is ok, but your BAL_Init and BAL_Load should return NULL if an error occured. That way, the caller can at least try to recover.
-
I'm missing a BAL_Toggle.
-
You should consider moving the struct definition into the implementation file. You only use pointers to it, so you can just as well make it opaque. That way, you can change the implementation (i.e. struct-layout) of the array without having to recompile everything that included the header.
Context
StackExchange Code Review Q#12981, answer score: 6
Revisions (0)
No revisions yet.