Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

C++ has always had the ability to zero initialize arrays in an object constructor. Most people are not aware of it an still use the verbose and error prone memset. Read the entry about zero initialization on cppreference. Also a related question on SO related question on SO. You can rewrite your constructor like this:

C++ has always had the ability to zero initialize arrays in an object constructor. Most people are not aware of it an still use the verbose and error prone memset. Read the entry about zero initialization on cppreference. Also a related question on SO. You can rewrite your constructor like this:

C++ has always had the ability to zero initialize arrays in an object constructor. Most people are not aware of it an still use the verbose and error prone memset. Read the entry about zero initialization on cppreference. Also a related question on SO. You can rewrite your constructor like this:

Clarified last point.
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89

Sure that code is just a usage sample, but still, there is no real need to dynamically allocate the object. The ability to declared objects by value is one of the nicest features of C++, so use it instead of a dynamic alloc when it makes sense.

// No need to 'new' it in this case. 
BitArray<10> bs;

Pointers/dynamic memory are normally only used when you need to extend the lifetime of an object beyond its scope of declaration. This is not the case in your example.

Not to mention, of course, that the object in your sample code is never deleted and thus leaks memory.

Sure that code is just a usage sample, but still, there is no real need to dynamically allocate the object. The ability to declared objects by value is one of the nicest features of C++, so use it instead of a dynamic alloc when it makes sense. Not to mention, of course, that the object is never deleted and thus leaks memory.

Sure that code is just a usage sample, but still, there is no real need to dynamically allocate the object. The ability to declared objects by value is one of the nicest features of C++, so use it instead of a dynamic alloc when it makes sense.

// No need to 'new' it in this case. 
BitArray<10> bs;

Pointers/dynamic memory are normally only used when you need to extend the lifetime of an object beyond its scope of declaration. This is not the case in your example.

Not to mention, of course, that the object in your sample code is never deleted and thus leaks memory.

added 83 characters in body
Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89

Prefer typed constants over #defines:

NO_OF_BITS should be a const unsigned int or const size_t and should also be a private member of BitArray, since it is not used elsewhere.

Also, NO_OF_BITS is a bad name for the constant. Num of bits in what? Num of bits in an integer. So named it accordingly.

As it was already suggested, use CHAR_BIT from <climits>. Also, adding unsigned to the sizeof call doesn't really make a difference.

The ALL_UPPERCASE notation should be reserved for macros and macro constants, to differentiate those from actual C++ entities (the preprocessor is like a mini-language inside C++ that doesn't live by the same rules). PascalCase or even camelCase are more modern notations for typed constants.

Summing up, it could look something like this:

 class BitArray {
 private:
 static const unsigned int IntBits = CHAR_BIT * sizeof(int);
 ...
 };

Side note: constexpr would have been my choice over const if this was C++11.

Use unsigned types if a value should never be negative:

ssize_t is the "signed" variant of size_t. You are using ssize_t as the template parameter for the size of your bit array. The message you pass to readers of your code with this is that the size of a bit array could be negative. I don't think this would make any sense. Either use std::size_t or a plain unsigned int.

Make consistent use of const and bool:

BitArray<n>::test() does not modify the bit array, so it should be a const method. E.g.:

int test(unsigned int) const;
 ^^^^^

The const will also go in the method definition.

Also, test() has no business returning an int. It is a true or false query, so it should return a bool.

You don't need memset to zero init an array:

C++ has always had the ability to zero initialize arrays in an object constructor. Most people are not aware of it an still use the verbose and error prone memset. Read the entry about zero initialization on cppreference. Also a related question on SO. You can rewrite your constructor like this:

template<ssize_t n>
BitArray<n>::BitArray()
 : array() 
 // zero initialized
{ }

Don't use dynamic allocations where a static will do:

BitArray<10>* bs = new BitArray<10>();

Sure that code is just a usage sample, but still, there is no real need to dynamically allocate the object. The ability to declared objects by value is one of the nicest features of C++, so use it instead of a dynamic alloc when it makes sense. Not to mention, of course, that the object is never deleted and thus leaks memory.

Prefer typed constants over #defines:

NO_OF_BITS should be a const unsigned int or const size_t and should also be a private member of BitArray, since it is not used elsewhere.

Also, NO_OF_BITS is a bad name for the constant. Num of bits in what? Num of bits in an integer. So named it accordingly.

As it was already suggested, use CHAR_BIT from <climits>. Also, adding unsigned to the sizeof call doesn't really make a difference.

The ALL_UPPERCASE notation should be reserved for macros and macro constants, to differentiate those from actual C++ entities (the preprocessor is like a mini-language inside C++ that doesn't live by the same rules). PascalCase or even camelCase are more modern notations for typed constants.

Summing up, it could look something like this:

 class BitArray {
 private:
 static const unsigned int IntBits = CHAR_BIT * sizeof(int);
 ...
 };

Side note: constexpr would have been my choice over const if this was C++11.

Use unsigned types if a value should never be negative:

ssize_t is the "signed" variant of size_t. You are using ssize_t as the template parameter for the size of your bit array. The message you pass to readers of your code with this is that the size of a bit array could be negative. I don't think this would make any sense. Either use std::size_t or a plain unsigned int.

Make consistent use of const and bool:

BitArray<n>::test() does not modify the bit array, so it should be a const method. E.g.:

int test(unsigned int) const;
 ^^^^^

The const will also go in the method definition.

Also, test() has no business returning an int. It is a true or false query, so it should return a bool.

You don't need memset to zero init an array:

C++ has always had the ability to zero initialize arrays in an object constructor. Most people are not aware of it an still use the verbose and error prone memset. Read the entry about zero initialization on cppreference. Also a related question on SO. You can rewrite your constructor like this:

template<ssize_t n>
BitArray<n>::BitArray()
 : array() 
 // zero initialized
{ }

Don't use dynamic allocations where a static will do:

BitArray<10>* bs = new BitArray<10>();

Sure that code is just a usage sample, but still, there is no real need to dynamically allocate the object. The ability to declared objects by value is one of the nicest features of C++, so use it instead of a dynamic alloc when it makes sense.

Prefer typed constants over #defines:

NO_OF_BITS should be a const unsigned int or const size_t and should also be a private member of BitArray, since it is not used elsewhere.

Also, NO_OF_BITS is a bad name for the constant. Num of bits in what? Num of bits in an integer. So named it accordingly.

As it was already suggested, use CHAR_BIT from <climits>. Also, adding unsigned to the sizeof call doesn't really make a difference.

The ALL_UPPERCASE notation should be reserved for macros and macro constants, to differentiate those from actual C++ entities (the preprocessor is like a mini-language inside C++ that doesn't live by the same rules). PascalCase or even camelCase are more modern notations for typed constants.

Summing up, it could look something like this:

 class BitArray {
 private:
 static const unsigned int IntBits = CHAR_BIT * sizeof(int);
 ...
 };

Side note: constexpr would have been my choice over const if this was C++11.

Use unsigned types if a value should never be negative:

ssize_t is the "signed" variant of size_t. You are using ssize_t as the template parameter for the size of your bit array. The message you pass to readers of your code with this is that the size of a bit array could be negative. I don't think this would make any sense. Either use std::size_t or a plain unsigned int.

Make consistent use of const and bool:

BitArray<n>::test() does not modify the bit array, so it should be a const method. E.g.:

int test(unsigned int) const;
 ^^^^^

The const will also go in the method definition.

Also, test() has no business returning an int. It is a true or false query, so it should return a bool.

You don't need memset to zero init an array:

C++ has always had the ability to zero initialize arrays in an object constructor. Most people are not aware of it an still use the verbose and error prone memset. Read the entry about zero initialization on cppreference. Also a related question on SO. You can rewrite your constructor like this:

template<ssize_t n>
BitArray<n>::BitArray()
 : array() 
 // zero initialized
{ }

Don't use dynamic allocations where a static will do:

BitArray<10>* bs = new BitArray<10>();

Sure that code is just a usage sample, but still, there is no real need to dynamically allocate the object. The ability to declared objects by value is one of the nicest features of C++, so use it instead of a dynamic alloc when it makes sense. Not to mention, of course, that the object is never deleted and thus leaks memory.

Source Link
glampert
  • 17.3k
  • 4
  • 31
  • 89
Loading
lang-cpp

AltStyle によって変換されたページ (->オリジナル) /