ben-strasser / fast-cpp-csv-parser

fast-cpp-csv-parser
BSD 3-Clause "New" or "Revised" License
2.11k stars 440 forks source link

what needs to be adjusted to make the code work without C++ 11 #59

Open yuhaim opened 6 years ago

yuhaim commented 6 years ago

Could you please move the instructions from google code here local in github? https://code.google.com/p/fast-cpp-csv-parser/issues/detail?id=6

For within some regions, we could not access services from Google =_=!

It seems OK after deleting constexpr and replacing std::snprintf() with fprintf()

ben-strasser commented 6 years ago

Hi,

below is a copy&paste dump from the discussion thread over on Google Code. Please note that the real solution to the problem is to use a compiler that supports at least C++ 11.

Best Regards Ben Strasser

What steps will reproduce the problem? 1. Build on windows with Visual Studio 2013

What is the expected output? What do you see instead? Compile is successful

What version of the product are you using? On what operating system? latest Windows

Please provide any additional information below. 1. Had to create a custom snprintf function since VS2013 still doesn't have one

Had to remove all the constexpr since VS2013 complains about those

Also VS2013 complains about multiple default constructors, this time it's seems right, these can either be chosen when no arguments are supplied CSVReader() = delete; explicit CSVReader(Args...args):in(std::forward<Args>(args)...){

I commented out the first one

Attached is the new file with all the patches Attachments

csv.h 27.35KB

Comment #1 Posted on Sep 22, 2014 by Happy Wombat

I had to do the same, which I did independently. Would be great if the author made the package VS2013 compatible. Comment #2 Posted on Sep 28, 2014 by Helpful Rabbit

Issues 2 & 3 could be solved by adjusting code. However, I'm not going to patch without issue 1 also being resolved. There is no point in making it semi-compatible. Everything or nothing.

Issue 1 is very problematic. I do not want to add a function named snprintf out of fear of having problems with other compilers. I do not want a complete independent function named my_snprintf because I want to use the compiler/stdlib provided function if it is available. I do not want "#ifdef this_compiler" because I do not want to keep track of what compiler has which features.

Further the patch has some severe problems: "#define snprintf std::snprintf" is a nogo as it does not play nice with namespaces. Suppose some other code in an unrelated header uses "std::snprintf". The macro would turn this into "std::std::snprintf" and you get some obscure error messages pointing to completely unrelated lines of code. This is very bad. Another problem is that a newer version of VS might support "snprintf" and then you "#ifdef _MSC_VER" breaks.

I added a link beside the download link to this issue. This way people using VS will not keep discovering the same issue but know from the beginning what needs to be done.

    Also VS2013 complains about multiple default constructors, this time it's seems right, these can either be chosen when no arguments are supplied

Actually I don't think VS is correct. CSVReader() = delete means that no default constructor should be generated, i.e., the code specifically states that there should be only one constructor. Obviously VS2013 generates some form of default constructor as otherwise it would not be ambiguous. Comment #3 Posted on Jan 14, 2015 by Quick Rabbit

An other fix suggestion for VS2013, IMHO less intruisive than the one suggested by the OP, but that defines a new function in the std namespace (and that is considered a bad practice). If std::snprintf is available by other means then define CSV_IO_MSVC_NO_SNPRINTF. if defined(_MSC_VER) && (_MSC_VER <= 1800) if !defined(constexpr) define constexpr endif if !defined(CSV_IO_MSVC_NO_SNPRINTF)

namespace std {

inline int c99_vsnprintf(char* str, size_t size, const char* format, va_list ap)
{
    int count = -1;

    if (size != 0)
        count = _vsnprintf_s(str, size, _TRUNCATE, format, ap);
    if (count == -1)
        count = _vscprintf(format, ap);

    return count;
}

inline int snprintf(char* str, size_t size, const char* format, ...)
{
    int count;
    va_list ap;

    va_start(ap, format);
    count = c99_vsnprintf(str, size, format, ap);
    va_end(ap);

    return count;
}

using namespace std;

} //namespace std

endif //CSV_IO_MSVC_NO_SNPRINTF endif // _MSC_VER

yuhaim commented 6 years ago

reminders: add #include remove using namespace std; inside

Mayitzin commented 6 years ago

Another addition I want to put is when compiling in Windows, especially using C++11 in VS.

When using the numeric limits on set_to_max_on_overflow, it will likely throw a warning that the max function has not enough arguments. This is because Windows defines max as a comparison Macro to get the largest of two values. To avoid this, I changed:

x = std::numeric_limits<T>::max();

To:

x = (std::numeric_limits<T>::max)();

Simply surrounding it with parenthesis will prevent the preprocessor to replace max() with the Windows Macro. Similarly with the min() definition:

x = (std::numeric_limits<T>::min)();

See: Syntax error with std::numeric_limits::max

ben-strasser commented 6 years ago

Can you provide a pull request where you enclose all std::numeric_limits::min/max? If you provide patches I can pull, as this issue is still relevant with modern VS.

urandon commented 4 years ago

It is an end-user responsibility to #define NOMINMAX before including Windows headers. It is not relevant to VS, it is just windows.h's conflicting macro.

ben-strasser commented 4 years ago

@urandon I guess, your comment is a reaction to the parenthesis added yesterday. This change was due to issue #101 . I agree with what you are saying. However, I am uncertain whether you suggest changing something to the current situation.

The reasoning for this change is that it helps some people without harming others. Now that I think about it, it might be more elegant to just use the macros from and avoid numeric_limits. However, I do not think that this difference really matters.