OpenXLSX icon indicating copy to clipboard operation
OpenXLSX copied to clipboard

Why std::string everywhere?

Open kelbon opened this issue 3 years ago • 6 comments

  1. I think it is possible to unicode name of sheet or smth? Isnt it?
  2. .cell("A:1") need to create string every time. May be allocation or smth, why you dont use string_views?

kelbon avatar Jul 01 '22 10:07 kelbon

Probably not a big deal, if you have the time to write "A1" (not even mentioning small string optimization) You'll use XLCell XLWorksheet::cell(uint32_t rowNumber, uint16_t columnNumber) const everywhere else.

reder2000 avatar Jul 01 '22 17:07 reder2000

Probably not a big deal, if you have the time to write "A1" (not even mentioning small string optimization) You'll use XLCell XLWorksheet::cell(uint32_t rowNumber, uint16_t columnNumber) const everywhere else.

And its big deal, if you dont want transform "A1:B2" to integers(its almost impossible in Excel AAB system) It also may be a good thing to just get cell range from string_view like "A1:B4" or smth (without exceptions, with something like std::expected) Also i think need a better way to 'visit' value of cell, why it is not a std::variant(in Cell.value() return type) ?

kelbon avatar Jul 01 '22 18:07 kelbon

Hi @kelbon

1. I think it is possible to unicode name of sheet or smth? Isnt it?

I'm not sure what you mean. Are you asking if it is possible to name worksheets with unicode strings, then yes:

XLDocument doc;
doc.create("./Spreadsheet.xlsx");
doc.workbook().addWorksheet("スプレッドシート");

Just make sure that your source code is also encoded in UTF-8 (see the README for details).

2. .cell("A:1") need to create string every time. May be allocation or smth, why you dont use string_views?

As @reder2000 rightly noted, you can use .cell(uint32_t rowNumber, uint16_t columnNumber) instead. Also, you can use an XLCellReference object instead, You can use either row/column numbers or address strings to create such an object, and it can be used to convert between the two, so it's quite handy.

Regarding the use of strings, as @reder2000 also mentions, the address strings will be so small that the compiler will use the Small String Optimization, so the performance penalty will be minimal. That being said, OpenXLSX does use std::string many places where std::string_view might be better. It is something that is on my todo-list, but it may be a bit tricky, because the std::string_view does not own the string, so I have to make sure that the source string is not deleted after returning a std::string_view.

Your comment about using a visitor pattern is a good one. Internally, the XLCellValue is actually a std::variant, but for various reasons, I had to wrap it in a separate object. But it's a good idea to enable an XLCellValue object to be 'visited' by a visitor of some sort.

troldal avatar Jul 03 '22 10:07 troldal

In really best realization it may be cell("A1:B4") which accepts consteval constructed object of RangeCell so it may be calculated even on compile time. And if you use const std::string&, then you always can use string_view as well, but if you need to copy string, just use std::string in signature(or std::u8string if it may be unicode)

kelbon avatar Jul 03 '22 11:07 kelbon

At least paths in functions like open should be replaced with something like e.g std::filesystem::path or boost::filesystem::path. to avoid any possible uncertainties considering for example windows using UTF-16.

JanczarKurek avatar Aug 06 '22 12:08 JanczarKurek