Skip to main content
Code Review

Return to Answer

Corrected several points in response to @Deduplicator's comment. Added a new point to address the pod vs. trivial issue.
Source Link
Ben Steffan
Ben Steffan
  • 5.3k
  • 1
  • 19
  • 35
  1. malloc and realloc live in cstdlib. malloc.h is not a Cstandard header, neither in C++ nor in C, and should not be included from a C++ programrelied on in any case.
  2. Also, you're missing #include <utility> for std::move.
  3. Legacy C functionality, as everything else from the standard library, lives in the std namespace. ::malloc, if you include the corresponding ::realloc<cheader>, files. While you can also include the C standard headers via ::size_t<header.h> etc, this is highly discouraged. With the first option, names may also exist in the global namespace, but that'sthis is not guaranteed and should not be relied on.
  4. Your malloc and realloc functions need not be inline. In fact, inline does nothing here; remove it.
  5. You don't need to check for null pointers when freeing memory. Passing a null pointer to free is always well defined and does nothing.
  6. You don't need to move out all the data members from other in your move constructor. All of those members are trivial to copy, and have no special move behavior.
  7. The move assignment operator invokes undefined behavior. You cannot just call the destructor of an object and then write to its members. Calling the destructor completely ends the lifetime of the object; afterwards, there's nothing there to write to. Furthermore, this is a kind of anti-pattern; in almost all cases, the move assignment operator should be implemented via the copy-and-swap idiom.
  8. push_back is broken. You call change_capacity with the result of std::min(m_capacity * 2, 1), which is either 0 or 1, in any case. Most likely, you'd want to use std::max instead.
  9. if (m_size + 1 > m_capacity) looks strange to my eyes. I would just write if (m_size >= m_capacity).
  10. pop_back should definitely return a value. It's not very useful otherwise.
  11. Since you're going for what seems to be a std-like container interface, you should add a little more functionality. For one, you're missing some of the usual type definitions, such as iterator and const_iterator. Furthermore, some sort of emplace mechanism would be appropriate, as well as some way to clear the container and some functionality to shrink it, as well as some other things.
  12. You should make sure that memory is properly initialized. As a user, I would expect that it's safe to access every element in bounds; however, you are not initializing memory at all, leaving that job to the user. I'd expect memory to be default- or zero-initialized; making use of std::calloc should do the trick (alternatively, you can also just std::memset the area).
  13. I would make your malloc and realloc functions a little bit more useful. As of right now, the onus of checking whether allocation succeeded is put on the user of those functions, but in my opinion it would be much cleaner and safer to lift those functions to a "C++-safe" level, i.e. make them check the return value of the underlying allocation call directly and convert that into an exception directly.
  14. std::is_pod is being deprecated, in accordance with the standard library moving away from the term POD as a whole. Also, the requirement that T must be a POD type is too strict; your vector will still work correctly if T is trivial.
  1. malloc and realloc live in cstdlib. malloc.h is a C header and should not be included from a C++ program.
  2. Also, you're missing #include <utility> for std::move.
  3. Legacy C functionality, as everything else from the standard library, lives in the std namespace. ::malloc, ::realloc, ::size_t etc. may also exist in the global namespace, but that's not guaranteed and should not be relied on.
  4. Your malloc and realloc functions need not be inline. In fact, inline does nothing here; remove it.
  5. You don't need to check for null pointers when freeing memory. Passing a null pointer to free is always well defined and does nothing.
  6. You don't need to move out all the data members from other in your move constructor. All of those members are trivial to copy, and have no special move behavior.
  7. The move assignment operator invokes undefined behavior. You cannot just call the destructor of an object and then write to its members. Calling the destructor completely ends the lifetime of the object; afterwards, there's nothing there to write to. Furthermore, this is a kind of anti-pattern; in almost all cases, the move assignment operator should be implemented via the copy-and-swap idiom.
  8. push_back is broken. You call change_capacity with the result of std::min(m_capacity * 2, 1), which is either 0 or 1, in any case. Most likely, you'd want to use std::max instead.
  9. if (m_size + 1 > m_capacity) looks strange to my eyes. I would just write if (m_size >= m_capacity).
  10. pop_back should definitely return a value. It's not very useful otherwise.
  11. Since you're going for what seems to be a std-like container interface, you should add a little more functionality. For one, you're missing some of the usual type definitions, such as iterator and const_iterator. Furthermore, some sort of emplace mechanism would be appropriate, as well as some way to clear the container and some functionality to shrink it, as well as some other things.
  12. You should make sure that memory is properly initialized. As a user, I would expect that it's safe to access every element in bounds; however, you are not initializing memory at all, leaving that job to the user. I'd expect memory to be default- or zero-initialized; making use of std::calloc should do the trick (alternatively, you can also just std::memset the area).
  13. I would make your malloc and realloc functions a little bit more useful. As of right now, the onus of checking whether allocation succeeded is put on the user of those functions, but in my opinion it would be much cleaner and safer to lift those functions to a "C++-safe" level, i.e. make them check the return value of the underlying allocation call directly and convert that into an exception directly.
  1. malloc and realloc live in cstdlib. malloc.h is not a standard header, neither in C++ nor in C, and should not be relied on in any case.
  2. Also, you're missing #include <utility> for std::move.
  3. Legacy C functionality, as everything else from the standard library, lives in the std namespace, if you include the corresponding <cheader> files. While you can also include the C standard headers via <header.h>, this is highly discouraged. With the first option, names may also exist in the global namespace, but this is not guaranteed and should not be relied on.
  4. Your malloc and realloc functions need not be inline. In fact, inline does nothing here; remove it.
  5. You don't need to check for null pointers when freeing memory. Passing a null pointer to free is always well defined and does nothing.
  6. You don't need to move out all the data members from other in your move constructor. All of those members are trivial to copy, and have no special move behavior.
  7. The move assignment operator invokes undefined behavior. You cannot just call the destructor of an object and then write to its members. Calling the destructor completely ends the lifetime of the object; afterwards, there's nothing there to write to. Furthermore, this is a kind of anti-pattern; in almost all cases, the move assignment operator should be implemented via the copy-and-swap idiom.
  8. push_back is broken. You call change_capacity with the result of std::min(m_capacity * 2, 1), which is either 0 or 1, in any case. Most likely, you'd want to use std::max instead.
  9. if (m_size + 1 > m_capacity) looks strange to my eyes. I would just write if (m_size >= m_capacity).
  10. pop_back should definitely return a value. It's not very useful otherwise.
  11. Since you're going for what seems to be a std-like container interface, you should add a little more functionality. For one, you're missing some of the usual type definitions, such as iterator and const_iterator. Furthermore, some sort of emplace mechanism would be appropriate, as well as some way to clear the container and some functionality to shrink it, as well as some other things.
  12. You should make sure that memory is properly initialized. As a user, I would expect that it's safe to access every element in bounds; however, you are not initializing memory at all, leaving that job to the user. I'd expect memory to be default- or zero-initialized; making use of std::calloc should do the trick (alternatively, you can also just std::memset the area).
  13. I would make your malloc and realloc functions a little bit more useful. As of right now, the onus of checking whether allocation succeeded is put on the user of those functions, but in my opinion it would be much cleaner and safer to lift those functions to a "C++-safe" level, i.e. make them check the return value of the underlying allocation call directly and convert that into an exception directly.
  14. std::is_pod is being deprecated, in accordance with the standard library moving away from the term POD as a whole. Also, the requirement that T must be a POD type is too strict; your vector will still work correctly if T is trivial.
Source Link
Ben Steffan
Ben Steffan
  • 5.3k
  • 1
  • 19
  • 35

  1. malloc and realloc live in cstdlib. malloc.h is a C header and should not be included from a C++ program.
  2. Also, you're missing #include <utility> for std::move.
  3. Legacy C functionality, as everything else from the standard library, lives in the std namespace. ::malloc, ::realloc, ::size_t etc. may also exist in the global namespace, but that's not guaranteed and should not be relied on.
  4. Your malloc and realloc functions need not be inline. In fact, inline does nothing here; remove it.
  5. You don't need to check for null pointers when freeing memory. Passing a null pointer to free is always well defined and does nothing.
  6. You don't need to move out all the data members from other in your move constructor. All of those members are trivial to copy, and have no special move behavior.
  7. The move assignment operator invokes undefined behavior. You cannot just call the destructor of an object and then write to its members. Calling the destructor completely ends the lifetime of the object; afterwards, there's nothing there to write to. Furthermore, this is a kind of anti-pattern; in almost all cases, the move assignment operator should be implemented via the copy-and-swap idiom.
  8. push_back is broken. You call change_capacity with the result of std::min(m_capacity * 2, 1), which is either 0 or 1, in any case. Most likely, you'd want to use std::max instead.
  9. if (m_size + 1 > m_capacity) looks strange to my eyes. I would just write if (m_size >= m_capacity).
  10. pop_back should definitely return a value. It's not very useful otherwise.
  11. Since you're going for what seems to be a std-like container interface, you should add a little more functionality. For one, you're missing some of the usual type definitions, such as iterator and const_iterator. Furthermore, some sort of emplace mechanism would be appropriate, as well as some way to clear the container and some functionality to shrink it, as well as some other things.
  12. You should make sure that memory is properly initialized. As a user, I would expect that it's safe to access every element in bounds; however, you are not initializing memory at all, leaving that job to the user. I'd expect memory to be default- or zero-initialized; making use of std::calloc should do the trick (alternatively, you can also just std::memset the area).
  13. I would make your malloc and realloc functions a little bit more useful. As of right now, the onus of checking whether allocation succeeded is put on the user of those functions, but in my opinion it would be much cleaner and safer to lift those functions to a "C++-safe" level, i.e. make them check the return value of the underlying allocation call directly and convert that into an exception directly.