atl24 cleanup for latest tools #434
Conversation
packages/core/FieldArray.h
Outdated
| @@ -160,7 +160,8 @@ FieldArray<T,N>::FieldArray(std::initializer_list<T> init_list): | |||
| *----------------------------------------------------------------------------*/ | |||
| template <class T, int N> | |||
| FieldArray<T,N>::FieldArray(void): | |||
| FieldUnsafeArray<T>(&values[0], N) | |||
| FieldUnsafeArray<T>(&values[0], N), | |||
| values() | |||
There was a problem hiding this comment.
I prefer to leave a value uninitialized when it is actually uninitialized. This allows address sanitizer / valgrind to catch uninitialized data access.
packages/core/Field.h
Outdated
|
|
||
| // encoding | ||
| template<class T> | ||
| inline uint32_t getImpliedEncoding(void) { | ||
| T dummy; | ||
| T dummy = T(); |
There was a problem hiding this comment.
This code is only used to set the encoding type of the field class and should not have data associated with it. By calling the initialization function of the type, the dummy variable is initialized, but then never used. I don't know how the compiler will treat a variable that has been written to and then ignored. My feeling (and I admit it is only that), is that keeping it uninitialized makes it unambiguous that there are no side effects to calling the function getImpliedEncoding, which means that it should just completely optimize away and be replaced with the encoding type.
packages/core/Field.h
Outdated
| inline uint32_t toEncoding(double& v) { (void)v; return Field::DOUBLE; }; | ||
| inline uint32_t toEncoding(time8_t& v) { (void)v; return Field::TIME8; }; | ||
| inline uint32_t toEncoding(string& v) { (void)v; return Field::STRING; }; | ||
| inline uint32_t toEncoding(const bool& v) { (void)v; return Field::BOOL; }; |
There was a problem hiding this comment.
When the getImpliedEncoding function calls these functions with a variable that is uninitialized, the compiler will complain if the variable is declared const because at that point the assumption is that it is read only and not set.
packages/core/FieldColumn.h
Outdated
| inline uint32_t toEncoding(FieldColumn<double>& v) { (void)v; return Field::NESTED_COLUMN | Field::DOUBLE; }; | ||
| inline uint32_t toEncoding(FieldColumn<time8_t>& v) { (void)v; return Field::NESTED_COLUMN | Field::TIME8; }; | ||
| inline uint32_t toEncoding(FieldColumn<string>& v) { (void)v; return Field::NESTED_COLUMN | Field::STRING; }; | ||
| inline uint32_t toEncoding(const FieldColumn<bool>& v) { (void)v; return Field::NESTED_COLUMN | Field::BOOL; }; |
packages/core/FieldColumn.h
Outdated
| @@ -282,6 +282,7 @@ FieldColumn<T>& FieldColumn<T>::operator= (const FieldColumn<T>& column) | |||
| } | |||
|
|
|||
| encoding = column.encoding; | |||
| chunkSize = column.chunkSize; | |||
There was a problem hiding this comment.
This is a bug. The chunkSize has to be the same as was used in the lines above when appending elements.
packages/core/FieldElement.h
Outdated
| @@ -73,7 +73,7 @@ class FieldElement: public Field | |||
| *--------------------------------------------------------------------*/ | |||
|
|
|||
| operator bool() const { | |||
| return value != 0; // Object is "true" if value is non-zero | |||
| return static_cast<T>(value) != 0; // Object is "true" if value is non-zero | |||
There was a problem hiding this comment.
I am very nervous about this change. The code, as it was, will allow the compiler to issue an error when it is inappropriately used. I think the new code would do the same, but I am not sure why there would ever be a reason to perform a static cast of a variable of type T to the exact same type that it already is.
The problem with this change, is that I know the original code will cause the compiler to issue an error when I do something that doesn't make sense, but I don't know how the new code will behave.
packages/core/FieldElement.h
Outdated
| @@ -102,7 +102,7 @@ FieldElement<T>::FieldElement(T default_value): | |||
| *----------------------------------------------------------------------------*/ | |||
| template <class T> | |||
| FieldElement<T>::FieldElement(): | |||
| Field(ELEMENT, getImpliedEncoding<T>()) | |||
| Field(ELEMENT, getImpliedEncoding<T>()), value() | |||
There was a problem hiding this comment.
I prefer to avoid artificially initializing variables because that prevents tools like valgrind and address sanitizer from catching my mistakes.
packages/core/FieldEnumeration.h
Outdated
| @@ -126,6 +126,7 @@ FieldEnumeration<T,N>::FieldEnumeration(std::initializer_list<bool> init_list): | |||
| template <class T, int N> | |||
| FieldEnumeration<T,N>::FieldEnumeration(void): | |||
| Field(ENUMERATION, getImpliedEncoding<T>()), | |||
| values(), | |||
packages/core/FieldList.h
Outdated
| @@ -203,7 +203,22 @@ long FieldList<T>::length(void) const | |||
| template<class T> | |||
| const Field* FieldList<T>::get(long i) const | |||
| { | |||
| return reinterpret_cast<const Field*>(&values[i]); | |||
There was a problem hiding this comment.
this code only makes sense when there is a list of fields; I want the code to crash if that is not the case.
No description provided.