Permalink
Browse files

Partial code review

Comments will be in line notes on github
  • Loading branch information...
Naja Melan
Naja Melan committed Nov 10, 2010
1 parent 98f731e commit b75b2ff4ce880c1acca4661f984eff87a8666347
Showing with 13 additions and 9 deletions.
  1. +13 −9 include/loki/SafeBits.h
View
@@ -171,7 +171,7 @@ class SafeBitConst
// can never be evaluated for i == 0? Some compilers see shift by ( i - 1 )
// and complain that for i == 0 the number is invalid, without
// checking that shift needs evaluating.
- return SafeBitConst( ( i > 0 ) ? ( word_t(1) << ( ( i > 0 ) ? ( i - 1 ) : 0 ) ) : 0 );
+ return SafeBitConst( ( i > 0 ) ? ( word_t(1) << ( i - 1 ) ) : 0 );

This comment has been minimized.

Show comment Hide comment
@najamelan

najamelan Nov 10, 2010

Owner

Ok, Im sorry about this one, only now I discovered the ridiculous warning C4293...

@najamelan

najamelan Nov 10, 2010

Owner

Ok, Im sorry about this one, only now I discovered the ridiculous warning C4293...

}
#else
static SafeBitConst make_bit_const( unsigned int i )
@@ -333,17 +333,17 @@ class SafeBitField
SafeBitField operator & ( const SafeBitField & rhs ) const { return SafeBitField( word & rhs.word ); }
SafeBitField operator ^ ( const SafeBitField & rhs ) const { return SafeBitField( word ^ rhs.word ); }
SafeBitField operator ~ ( void ) const { return SafeBitField( ~word ); }
- SafeBitField operator |= ( const SafeBitField & rhs ) { word |= rhs.word; return SafeBitField( *this ); }
- SafeBitField operator &= ( const SafeBitField & rhs ) { word &= rhs.word; return SafeBitField( *this ); }
- SafeBitField operator ^= ( const SafeBitField & rhs ) { word ^= rhs.word; return SafeBitField( *this ); }
+ SafeBitField operator |= ( const SafeBitField & rhs ) { word |= rhs.word; return *this; }
+ SafeBitField operator &= ( const SafeBitField & rhs ) { word &= rhs.word; return *this; }
+ SafeBitField operator ^= ( const SafeBitField & rhs ) { word ^= rhs.word; return *this; }

This comment has been minimized.

Show comment Hide comment
@najamelan

najamelan Nov 10, 2010

Owner

I might be mistaken, and if I am, please teach me something, but I really don't understand the meaning of calling your constructor on your this pointer. The copy constructor is not explicit.

It also strikes me as odd that assignment operators don't return by reference.

@najamelan

najamelan Nov 10, 2010

Owner

I might be mistaken, and if I am, please teach me something, but I really don't understand the meaning of calling your constructor on your this pointer. The copy constructor is not explicit.

It also strikes me as odd that assignment operators don't return by reference.

/// Bitwise operators that use bit-constants.
SafeBitField operator | ( const_t rhs ) const { return SafeBitField( word | rhs.word ); }
SafeBitField operator & ( const_t rhs ) const { return SafeBitField( word & rhs.word ); }
SafeBitField operator ^ ( const_t rhs ) const { return SafeBitField( word ^ rhs.word ); }
- SafeBitField operator |= ( const_t rhs ) { word |= rhs.word; return SafeBitField( *this ); }
- SafeBitField operator &= ( const_t rhs ) { word &= rhs.word; return SafeBitField( *this ); }
- SafeBitField operator ^= ( const_t rhs ) { word ^= rhs.word; return SafeBitField( *this ); }
+ SafeBitField operator |= ( const_t rhs ) { word |= rhs.word; return *this; }
+ SafeBitField operator &= ( const_t rhs ) { word &= rhs.word; return *this; }
+ SafeBitField operator ^= ( const_t rhs ) { word ^= rhs.word; return *this; }
// Conversion to bool.
// This is a major source of headaches, but it's required to support code like this:
@@ -471,7 +471,11 @@ inline SafeBitField< unique_index, word_t > operator != ( bool, SafeBitField< un
// This creates a typedef field_t for SafeBitField<unique_index, ulong> where index is the current line number. Since line numbers __LINE__ are counted
// separately for all header files, this ends up being the same type in all files using the header which defines field_t.
#ifdef LOKI_SAFE_BIT_FIELD

This comment has been minimized.

Show comment Hide comment
@najamelan

najamelan Nov 10, 2010

Owner

I would suggest you enable the code by default and don't put a #define in the library, rather use #ifndef LOKI_NO_SAFE_BIT_FIELD to allow a user to disable the templates if needed.

This has the advantage that the user can disable them if they need, potentially with compiler checks without having to change the Loki library code.

@najamelan

najamelan Nov 10, 2010

Owner

I would suggest you enable the code by default and don't put a #define in the library, rather use #ifndef LOKI_NO_SAFE_BIT_FIELD to allow a user to disable the templates if needed.

This has the advantage that the user can disable them if they need, potentially with compiler checks without having to change the Loki library code.

- #define LOKI_BIT_FIELD( word_t ) typedef SafeBitField<__LINE__, word_t>
+ #ifdef __COUNTER__
+ #define LOKI_BIT_FIELD( word_t ) typedef Loki::SafeBitField<__COUNTER__, word_t>
+ #else
+ #define LOKI_BIT_FIELD( word_t ) typedef Loki::SafeBitField<__LINE__, word_t>
+ #endif
#else

This comment has been minimized.

Show comment Hide comment
@najamelan

najamelan Nov 10, 2010

Owner

a. This does not compile out of the box, because SafeBitField is in the namespace Loki:: so unless the calling code has "using Loki;" it cannot ever work.
b. __LINE__ is not an optimal choice for a unique template for several reasons.
1. It is not always unique. If someone puts two declarations on the same line you're in trouble, but also if two included headers use this macro on the same line it fails.
That is not a true disaster, it will be rare, but it is bound to happen sometimes.
2. it does not compile on msvc with the default settings, because switch /ZI blows the __LINE__ macro. http://support.microsoft.com/kb/199057

I have not found a satisfying solution to this problem.

I have now found that the proposed solution with __COUNTER__ does not work because the linker requires the types to be identical across translation units. This kind of means on msvc user are forced to change their project setting if they want to use these safebits.

@najamelan

najamelan Nov 10, 2010

Owner

a. This does not compile out of the box, because SafeBitField is in the namespace Loki:: so unless the calling code has "using Loki;" it cannot ever work.
b. __LINE__ is not an optimal choice for a unique template for several reasons.
1. It is not always unique. If someone puts two declarations on the same line you're in trouble, but also if two included headers use this macro on the same line it fails.
That is not a true disaster, it will be rare, but it is bound to happen sometimes.
2. it does not compile on msvc with the default settings, because switch /ZI blows the __LINE__ macro. http://support.microsoft.com/kb/199057

I have not found a satisfying solution to this problem.

I have now found that the proposed solution with __COUNTER__ does not work because the linker requires the types to be identical across translation units. This kind of means on msvc user are forced to change their project setting if they want to use these safebits.

#define LOKI_BIT_FIELD( word_t ) typedef word_t
#endif // LOKI_SAFE_BIT_FIELD
@@ -488,7 +492,7 @@ inline SafeBitField< unique_index, word_t > operator != ( bool, SafeBitField< un
static const field_t::const_t label = field_t::const_t::make_bit_const( bit_index )
#endif // LOKI_BIT_FIELD_NONTEMPLATE_INIT
#else
- inline size_t make_bit_const( size_t i ) { return ( i > 0 ) ? ( size_t(1) << ( ( i > 0 ) ? ( i - 1 ) : 0 ) ) : 0; }
+ inline size_t make_bit_const( size_t i ) { return ( i > 0 ) ? ( size_t(1) << ( i - 1 ) ) : 0; }
#define LOKI_BIT_CONST( field_t, label, bit_index ) static const field_t label = make_bit_const( bit_index )
#endif // LOKI_SAFE_BIT_FIELD

0 comments on commit b75b2ff

Please sign in to comment.