Skip to main content
added 126 characters in body
Source Link
Deduplicator
  • 19.3k
  • 1
  • 31
  • 65
  1. Debug-code

    1. The proper destination for debug-output is STDERRstd::clog (which goes to STDERR by default, std::cerr has the same destination but no buffering), not STDOUTstd::cout (which goes to STDOUT). Doubly so for trace-output.

    2. I guess you resort to std::endl because you use the wrong stream, though that's still a poor excuse. If you need an explicit flush, be explicit and use std::flush. Don't leave us wondering if you just wanted a newline.

    3. Get rid of all the conditional compilation using the preprocessor.
      While it might make sense to allow tracing your calls, code it similar to assert() or at least get rid of it at the end. Also, don't hang it on NDEBUG, tracing is far more intrusive.

    // trace.h
    #ifdef TRACE
    #undef trace
    #include <iostream>
    template<class...Ts>
    inline void trace(Ts const&... ts) noexcept {
        using T = int[];
        T{0, (std::cerr << ts, void(), 0)};
    }
    #define trace(...) trace(__VA_ARGS__)
    #else
    #define trace(...) void()
    #endif
    
    
  2. Model your type on the standard library as good as you can. An interface should be as unsurprising as it can be without burdening the implementation unduly.

    1. Put the templates type-argument first. Everyone else does it.
    2. Use a std::size_t for the size like everyone else.
      Also, if you don't want to name it N, use something more descriptive like CAPACITY.
    3. Allow initialization from arbitrary iterator-pairs. That can be leveraged for std::initializer_list.
    4. Support move- and copy-insertion for .push_back(). The easiest way is by supporting .emplace_back(), as you don't need to special-case copying an element and re-allocation.
    5. For .insert(), create and delegate to .emplace().
    6. .pop_back() should not return anything, because copying the removed element can be a costly waste of time.
    7. operator[] needs a constant overload, and should not catch errors. Adding an assert() would be appropriate though.
      .at() is the member which should check bounds.
    8. .back() also misses its constant overload, and should at most contain an assert() for checking errors.
      Without .front() it seems lonely.
    9. You are missing the whole iterator-interface, among others. At least add a note: // TODO: iterators and more.
  3. Polishing the interface

    1. Just use = default; instead of {} for the default ctor. Some code checks for trivial ctors and provides an optimized path.

    2. Use noexcept where expected. Doing so allows the compiler to remove exception-handling code, and there are often faster correct paths for code which cannot throw.

  4. Working with raw memory

    1. Don't initialize a bunch of objects you might never need. Use an unnamed union for the data-array to suppress the compiler calling special functions.

      union { T internal_buffer[BUFFER_SIZE]; };
      
    2. Yes, the above point means that the default copy- and move- ctor/assignment as well as the default dtor do the wrong thing. Define them yourself to do the right thing.

    3. Even if you do not want to use the Uninitialized memory algorithms, you can use placement-new and manual destructor-invocation directly. Just include <new>.

  5. (void) is the proper parameter-list for a no argument function in C, due to back-compatibility. While C++ accepts it, () is preferred.

  1. Debug-code

    1. The proper destination for debug-output is STDERR, not STDOUT. Doubly so for trace-output.

    2. I guess you resort to std::endl because you use the wrong stream, though that's still a poor excuse. If you need an explicit flush, be explicit and use std::flush. Don't leave us wondering if you just wanted a newline.

    3. Get rid of all the conditional compilation using the preprocessor.
      While it might make sense to allow tracing your calls, code it similar to assert() or at least get rid of it at the end. Also, don't hang it on NDEBUG, tracing is far more intrusive.

    // trace.h
    #ifdef TRACE
    #undef trace
    #include <iostream>
    template<class...Ts>
    inline void trace(Ts const&... ts) noexcept {
        using T = int[];
        T{0, (std::cerr << ts, void(), 0)};
    }
    #define trace(...) trace(__VA_ARGS__)
    #else
    #define trace(...) void()
    #endif
    
    
  2. Model your type on the standard library as good as you can. An interface should be as unsurprising as it can be without burdening the implementation unduly.

    1. Put the templates type-argument first. Everyone else does it.
    2. Use a std::size_t for the size like everyone else.
      Also, if you don't want to name it N, use something more descriptive like CAPACITY.
    3. Allow initialization from arbitrary iterator-pairs. That can be leveraged for std::initializer_list.
    4. Support move- and copy-insertion for .push_back(). The easiest way is by supporting .emplace_back(), as you don't need to special-case copying an element and re-allocation.
    5. For .insert(), create and delegate to .emplace().
    6. .pop_back() should not return anything, because copying the removed element can be a costly waste of time.
    7. operator[] needs a constant overload, and should not catch errors. Adding an assert() would be appropriate though.
      .at() is the member which should check bounds.
    8. .back() also misses its constant overload, and should at most contain an assert() for checking errors.
      Without .front() it seems lonely.
    9. You are missing the whole iterator-interface, among others. At least add a note: // TODO: iterators and more.
  3. Polishing the interface

    1. Just use = default; instead of {} for the default ctor. Some code checks for trivial ctors and provides an optimized path.

    2. Use noexcept where expected. Doing so allows the compiler to remove exception-handling code, and there are often faster correct paths for code which cannot throw.

  4. Working with raw memory

    1. Don't initialize a bunch of objects you might never need. Use an unnamed union for the data-array to suppress the compiler calling special functions.

      union { T internal_buffer[BUFFER_SIZE]; };
      
    2. Yes, the above point means that the default copy- and move- ctor/assignment as well as the default dtor do the wrong thing. Define them yourself to do the right thing.

    3. Even if you do not want to use the Uninitialized memory algorithms, you can use placement-new and manual destructor-invocation directly. Just include <new>.

  5. (void) is the proper parameter-list for a no argument function in C, due to back-compatibility. While C++ accepts it, () is preferred.

  1. Debug-code

    1. The proper destination for debug-output is std::clog (which goes to STDERR by default, std::cerr has the same destination but no buffering), not std::cout (which goes to STDOUT). Doubly so for trace-output.

    2. I guess you resort to std::endl because you use the wrong stream, though that's still a poor excuse. If you need an explicit flush, be explicit and use std::flush. Don't leave us wondering if you just wanted a newline.

    3. Get rid of all the conditional compilation using the preprocessor.
      While it might make sense to allow tracing your calls, code it similar to assert() or at least get rid of it at the end. Also, don't hang it on NDEBUG, tracing is far more intrusive.

    // trace.h
    #ifdef TRACE
    #undef trace
    #include <iostream>
    template<class...Ts>
    inline void trace(Ts const&... ts) noexcept {
        using T = int[];
        T{0, (std::cerr << ts, void(), 0)};
    }
    #define trace(...) trace(__VA_ARGS__)
    #else
    #define trace(...) void()
    #endif
    
    
  2. Model your type on the standard library as good as you can. An interface should be as unsurprising as it can be without burdening the implementation unduly.

    1. Put the templates type-argument first. Everyone else does it.
    2. Use a std::size_t for the size like everyone else.
      Also, if you don't want to name it N, use something more descriptive like CAPACITY.
    3. Allow initialization from arbitrary iterator-pairs. That can be leveraged for std::initializer_list.
    4. Support move- and copy-insertion for .push_back(). The easiest way is by supporting .emplace_back(), as you don't need to special-case copying an element and re-allocation.
    5. For .insert(), create and delegate to .emplace().
    6. .pop_back() should not return anything, because copying the removed element can be a costly waste of time.
    7. operator[] needs a constant overload, and should not catch errors. Adding an assert() would be appropriate though.
      .at() is the member which should check bounds.
    8. .back() also misses its constant overload, and should at most contain an assert() for checking errors.
      Without .front() it seems lonely.
    9. You are missing the whole iterator-interface, among others. At least add a note: // TODO: iterators and more.
  3. Polishing the interface

    1. Just use = default; instead of {} for the default ctor. Some code checks for trivial ctors and provides an optimized path.

    2. Use noexcept where expected. Doing so allows the compiler to remove exception-handling code, and there are often faster correct paths for code which cannot throw.

  4. Working with raw memory

    1. Don't initialize a bunch of objects you might never need. Use an unnamed union for the data-array to suppress the compiler calling special functions.

      union { T internal_buffer[BUFFER_SIZE]; };
      
    2. Yes, the above point means that the default copy- and move- ctor/assignment as well as the default dtor do the wrong thing. Define them yourself to do the right thing.

    3. Even if you do not want to use the Uninitialized memory algorithms, you can use placement-new and manual destructor-invocation directly. Just include <new>.

  5. (void) is the proper parameter-list for a no argument function in C, due to back-compatibility. While C++ accepts it, () is preferred.

Source Link
Deduplicator
  • 19.3k
  • 1
  • 31
  • 65

  1. Debug-code

    1. The proper destination for debug-output is STDERR, not STDOUT. Doubly so for trace-output.

    2. I guess you resort to std::endl because you use the wrong stream, though that's still a poor excuse. If you need an explicit flush, be explicit and use std::flush. Don't leave us wondering if you just wanted a newline.

    3. Get rid of all the conditional compilation using the preprocessor.
      While it might make sense to allow tracing your calls, code it similar to assert() or at least get rid of it at the end. Also, don't hang it on NDEBUG, tracing is far more intrusive.

    // trace.h
    #ifdef TRACE
    #undef trace
    #include <iostream>
    template<class...Ts>
    inline void trace(Ts const&... ts) noexcept {
        using T = int[];
        T{0, (std::cerr << ts, void(), 0)};
    }
    #define trace(...) trace(__VA_ARGS__)
    #else
    #define trace(...) void()
    #endif
    
    
  2. Model your type on the standard library as good as you can. An interface should be as unsurprising as it can be without burdening the implementation unduly.

    1. Put the templates type-argument first. Everyone else does it.
    2. Use a std::size_t for the size like everyone else.
      Also, if you don't want to name it N, use something more descriptive like CAPACITY.
    3. Allow initialization from arbitrary iterator-pairs. That can be leveraged for std::initializer_list.
    4. Support move- and copy-insertion for .push_back(). The easiest way is by supporting .emplace_back(), as you don't need to special-case copying an element and re-allocation.
    5. For .insert(), create and delegate to .emplace().
    6. .pop_back() should not return anything, because copying the removed element can be a costly waste of time.
    7. operator[] needs a constant overload, and should not catch errors. Adding an assert() would be appropriate though.
      .at() is the member which should check bounds.
    8. .back() also misses its constant overload, and should at most contain an assert() for checking errors.
      Without .front() it seems lonely.
    9. You are missing the whole iterator-interface, among others. At least add a note: // TODO: iterators and more.
  3. Polishing the interface

    1. Just use = default; instead of {} for the default ctor. Some code checks for trivial ctors and provides an optimized path.

    2. Use noexcept where expected. Doing so allows the compiler to remove exception-handling code, and there are often faster correct paths for code which cannot throw.

  4. Working with raw memory

    1. Don't initialize a bunch of objects you might never need. Use an unnamed union for the data-array to suppress the compiler calling special functions.

      union { T internal_buffer[BUFFER_SIZE]; };
      
    2. Yes, the above point means that the default copy- and move- ctor/assignment as well as the default dtor do the wrong thing. Define them yourself to do the right thing.

    3. Even if you do not want to use the Uninitialized memory algorithms, you can use placement-new and manual destructor-invocation directly. Just include <new>.

  5. (void) is the proper parameter-list for a no argument function in C, due to back-compatibility. While C++ accepts it, () is preferred.