tfussell / xlnt

:bar_chart: Cross-platform user-friendly xlsx library for C++11+
Other
1.5k stars 423 forks source link

string problems? Many compile errors #20

Closed Malvineous closed 9 years ago

Malvineous commented 9 years ago

Hi again,

I just updated to the latest git but I'm getting a bunch of xlnt::string errors in my code now:

cannot convert ‘xlnt::cell_reference::to_string() const()’ (type ‘xlnt::string’) to type ‘const unsigned char*’
        << row[xxx].get_reference().to_string()

note:   ‘xlnt::string’ is not derived from ‘const otl_value<TData>’

no known conversion for argument 2 from ‘xlnt::string’ to ‘const IField*’

no known conversion for argument 2 from ‘xlnt::string’ to ‘const current_time&’

error: no match for ‘operator<<’ (operand types are ‘std::basic_ostream<char>’ and ‘xlnt::string’)

/usr/include/c++/5.1.0/bits/basic_string.h:5167:5: note:   template argument deduction/substitution failed:
note:   ‘xlnt::string’ is not derived from ‘const std::basic_string<_CharT, _Traits, _Alloc>’

Is it no longer possible to use std::string with xlnt?

tfussell commented 9 years ago

Hey there. Ever since you mentioned allowing this library to be linked as a shared library, I discovered that standard library types can't be safely passed across the dll boundary. For this reason, I wrote xlnt::string which is similar to std::string. If you know that your executable and xlnt were built with the same compiler and settings, you can define XLNT_STD_STRING which allows you to convert std::string to xlnt::string with xlnt::to(), xlnt::string::from(), and xlnt::string (const std::string &). Otherwise, you can convert using implicit constructors using the data method and const char *: xlnt::string str = std_str.data() or std::string str = xlnt_str.data(). I'm sorry if this causes problems for your existing code. It was the only way I could find to solve this problem.

Malvineous commented 9 years ago

Oh right - sorry I'm not that familiar with how it works with DLLs - under Linux it isn't a problem as everything is always built with the same compiler.

Looking into it I see what you mean. However according to that, it looks like any C++ class can't be passed across a DLL boundary if the compilers differ, so using xlnt::string is not much different to std::string. (At least with std::string the user's compiler, the xlnt compiler and the C++ library compiler must be the same. With xlnt::string only the xlnt compiler and the user's compiler need to match.)

However if this is something you want to address, presumably you'll also have to create xlnt::vector because many of the classes currently pass around std::vector objects which will suffer from the same problem as std::string. Some functions also take std::istream references or std::pair so it's probably going to be an uphill battle!

Sorry to be so disagreeable after you've put in so much effort creating the xlnt::string class, but I think it makes the library that much easier to work with if you can keep to standard strings. I guess you could either disable .dll support for Windows by default (since it works fine under Linux) or just make a note that like every other C++ library, you should compile it yourself if you want to use the .dll version with your program, unless you're using Visual Studio version xyz (whatever you use to compile the .dll files you intend to distribute.)

What do you think?

tfussell commented 9 years ago

You make some good points. I guess I went into these changes without giving it enough consideration. You've convinced me that the best policy will be to revert to std::string and leave it to the user to ensure that they are using the library correctly.

The most recent commit, 3665334df96cb90c5bf9c1f18bc8b703bcd0bff8, should reflect these changes. I've silenced the warnings about crossing the DLL boundary in MSVC builds, and all tests are passing on all platforms again.

It's always good practice reimplementing standard library classes like string. Maybe xlnt::string will find some use in the future.