vincentlaucsb / csv-parser

A high-performance, fully-featured CSV parser and serializer for modern C++.
MIT License
901 stars 150 forks source link

CSVReader move operator should be either deleted or implemented #153

Closed alandefreitas closed 3 years ago

alandefreitas commented 3 years ago

Error

.../csv-parser-src/include/internal/csv_reader.hpp:136:20: warning: explicitly defaulted move assignment operator is implicitly deleted [-Wdefaulted-function-deleted]
        CSVReader& operator=(CSVReader&& other) = default;
                   ^
.../csv-parser-src/include/internal/csv_reader.hpp:202:23: note: move assignment operator of 'CSVReader' is implicitly deleted because field 'records' has a deleted move assignment operator
        RowCollection records = RowCollection(100);
                      ^
.../csv-parser-src/include/internal/basic_csv_parser.hpp:177:24: note: copy assignment operator of 'ThreadSafeDeque<csv::CSVRow>' is implicitly deleted because field '_lock' has a deleted copy assignment operator
            std::mutex _lock;
BowenXiao1999 commented 3 years ago

Got similar warning here.

vincentlaucsb commented 3 years ago

@BowenXiao1999 @alandefreitas What compiler and what version were you using?

alandefreitas commented 3 years ago

I'm using Clang 11 and GCC 10, but this should be an error in any compiler because it's against the standard. CSVReader can't have a default move assignment operator because records doesn't have a default move assignment operator. No compiler would know what to do.

You can either implement

CSVReader& operator=(CSVReader&& other) { // implement }

or just delete it

CSVReader& operator=(CSVReader&& other) = delete;

Deleting it should give you the same behavior as before because the operator already doesn't exist. At most, your compiler didn't warn you about it because it wasn't tested.

You are probably developing with the warnings turned off. When developing, you can create an option to show you the warnings by default in debug mode with something like:

if (CMAKE_BUILD_TYPE STREQUAL "Debug")
    set(DEBUG_MODE ON)
else ()
    set(DEBUG_MODE OFF)
endif ()

option(BUILD_WITH_WARNINGS "Use pedantic warnings. Developers should leave this ON." ${DEBUG_MODE})

# after creating the target:
if (BUILD_WITH_WARNINGS)
    if (MSVC)
        target_compile_options(csv INTERFACE /W4 /WX)
    else()
        target_compile_options(csv INTERFACE -Wall -Wextra -pedantic -Werror)
    endif()
endif()

This should help you eliminate problems.

Because of how C++ handles includes, unfixed warnings in header files are particularly problematic because these errors would propagate to all dependent projects, which would then be forced to turn off their own warnings or just not use the library.

The same problem applies to std::mutex _lock, because copying a lock state by default doesn't make much sense, so they preferred to delete the copy operators. The common pattern here is to create a new free lock for the new object, instead of trying to copy the state old lock.

vincentlaucsb commented 3 years ago

Should be fixed by #160 160