From 322490b397836614e6859a08a0ad8f9bccb64808 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:29:26 +1200 Subject: [PATCH 01/38] re-write workbook::reorder_relationships Issue #279 --- source/workbook/workbook.cpp | 151 ++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 75 deletions(-) diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 55c460c4..9ad48973 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -28,16 +28,6 @@ #include #include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include #include #include #include @@ -61,12 +51,22 @@ #include #include #include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include namespace { using xlnt::detail::open_stream; -template +template std::vector keys(const std::vector> &container) { auto result = std::vector(); @@ -80,7 +80,7 @@ std::vector keys(const std::vector> &container) return result; } -template +template bool contains(const std::vector> &container, const T key) { for (const auto &iter : container) @@ -451,29 +451,29 @@ workbook workbook::empty() stylesheet.parent = &wb; auto default_border = border() - .side(border_side::bottom, border::border_property()) - .side(border_side::top, border::border_property()) - .side(border_side::start, border::border_property()) - .side(border_side::end, border::border_property()) - .side(border_side::diagonal, border::border_property()); + .side(border_side::bottom, border::border_property()) + .side(border_side::top, border::border_property()) + .side(border_side::start, border::border_property()) + .side(border_side::end, border::border_property()) + .side(border_side::diagonal, border::border_property()); wb.d_->stylesheet_.get().borders.push_back(default_border); auto default_fill = fill(pattern_fill() - .type(pattern_fill_type::none)); + .type(pattern_fill_type::none)); stylesheet.fills.push_back(default_fill); auto gray125_fill = pattern_fill() - .type(pattern_fill_type::gray125); + .type(pattern_fill_type::gray125); stylesheet.fills.push_back(gray125_fill); auto default_font = font() - .name("Calibri") - .size(12) - .scheme("minor") - .family(2) - .color(theme_color(1)); + .name("Calibri") + .size(12) + .scheme("minor") + .family(2) + .color(theme_color(1)); stylesheet.fonts.push_back(default_font); - wb.create_builtin_style(0) + wb.create_builtin_style(0) .border(default_border) .fill(default_fill) .font(default_font) @@ -550,7 +550,7 @@ void workbook::register_package_part(relationship_type type) void workbook::register_workbook_part(relationship_type type) { auto wb_rel = manifest().relationship(path("/"), relationship_type::office_document); - auto wb_path = manifest().canonicalize({ wb_rel }); + auto wb_path = manifest().canonicalize({wb_rel}); if (!manifest().has_relationship(wb_path, type)) { @@ -1025,7 +1025,8 @@ void workbook::remove_sheet(worksheet ws) for (auto &title_rel_id_pair : d_->sheet_title_rel_id_map_) { title_rel_id_pair.second = rel_id_map.count(title_rel_id_pair.second) > 0 - ? rel_id_map[title_rel_id_pair.second] : title_rel_id_pair.second; + ? rel_id_map[title_rel_id_pair.second] + : title_rel_id_pair.second; } update_sheet_properties(); @@ -1390,7 +1391,7 @@ style workbook::create_style(const std::string &name) style workbook::create_builtin_style(const std::size_t builtin_id) { - return d_->stylesheet_.get().create_builtin_style(builtin_id); + return d_->stylesheet_.get().create_builtin_style(builtin_id); } style workbook::style(const std::string &name) @@ -1563,69 +1564,69 @@ void workbook::update_sheet_properties() } } -void workbook::reorder_relationships() +namespace { +// true if a sheet index is != worksheet relationship index +bool needs_reorder(const std::unordered_map &title_to_rels, + const std::vector &titles, + std::vector &relation_ids) { - const auto wb_rel = manifest().relationship(path("/"), relationship_type::office_document); - const auto wb_path = wb_rel.target().path(); - const auto relationships = manifest().relationships(wb_path); - std::unordered_map rel_map; - const auto titles = sheet_titles(); - const auto title_rel_id_map = d_->sheet_title_rel_id_map_; - bool needs_reorder = false; - - for (const auto &rel : relationships) + bool all_match = true; + for (std::size_t title_index = 0; title_index < titles.size(); ++title_index) { - rel_map[rel.id()] = rel; - - if (rel.type() == relationship_type::worksheet) + const auto &rel = title_to_rels.at(titles[title_index]); + relation_ids.push_back(rel); + const auto expected_rel_id = "rId" + std::to_string(title_index + 1); + if (rel != expected_rel_id) { - for (auto title_index = std::size_t(0); title_index < titles.size(); ++title_index) - { - const auto title = titles[title_index]; - - if (title_rel_id_map.at(title) == rel.id()) - { - const auto expected_rel_id = "rId" + std::to_string(title_index + 1); - - if (expected_rel_id != rel.id()) - { - needs_reorder = true; - } - - break; - } - } + all_match = false; } } + return !all_match; // if all are as expected, reorder not required +}; +} // namespace - if (!needs_reorder) +void workbook::reorder_relationships() +{ + const auto titles = sheet_titles(); + // the relation ID corresponding to the title at the same index is copied into here + std::vector worksheet_rel_ids; + worksheet_rel_ids.reserve(titles.size()); + if (!needs_reorder(d_->sheet_title_rel_id_map_, titles, worksheet_rel_ids)) { return; } - - for (const auto &rel : relationships) + // copy of existing relations + const auto wb_rel = manifest().relationship(path("/"), relationship_type::office_document); + const auto wb_path = wb_rel.target().path(); + auto rel_copy = manifest().relationships(wb_path); + // clear existing relations + for (const auto &rel : rel_copy) { manifest().unregister_relationship(uri(wb_path.string()), rel.id()); } - - for (auto index = std::size_t(0); index < rel_map.size(); ++index) + // create new relations + std::size_t index = 0; + auto new_id = [&index]() { return "rId" + std::to_string(++index); }; // ids start from 1 + // worksheets first + while (index < worksheet_rel_ids.size()) { - auto rel_id = "rId" + std::to_string(index + 1); - auto old_rel_id = std::string(); + auto rel_it = std::find_if(rel_copy.begin(), rel_copy.end(), + [&](const relationship &rel) { return rel.id() == worksheet_rel_ids[index]; }); - if (index < titles.size()) + std::string rel_id = new_id(); + d_->sheet_title_rel_id_map_.at(titles[index - 1]) = rel_id; // update title -> relation mapping + manifest().register_relationship(relationship(rel_id, rel_it->type(), + rel_it->source(), rel_it->target(), rel_it->target_mode())); + } + // then all the other relations in the same order they started (just new indices) + for (const auto& old_rel : rel_copy) + { + if (old_rel.type() == relationship_type::worksheet) { - auto title = titles[index]; - old_rel_id = title_rel_id_map.at(title); - d_->sheet_title_rel_id_map_[title] = rel_id; - } else { - old_rel_id = "rId" + std::to_string(index - titles.size() + 2); + continue; } - - auto old_rel = rel_map[old_rel_id]; - auto new_rel = relationship(rel_id, old_rel.type(), - old_rel.source(), old_rel.target(), old_rel.target_mode()); - manifest().register_relationship(new_rel); + manifest().register_relationship(relationship(new_id(), old_rel.type(), + old_rel.source(), old_rel.target(), old_rel.target_mode())); } } From cf8991a234d6ae1fba968081f01492a7309c2467 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sun, 8 Jul 2018 15:44:12 +1200 Subject: [PATCH 02/38] no need to recreate the uri --- source/workbook/workbook.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 9ad48973..4ac1a4d2 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -1596,13 +1596,12 @@ void workbook::reorder_relationships() return; } // copy of existing relations - const auto wb_rel = manifest().relationship(path("/"), relationship_type::office_document); - const auto wb_path = wb_rel.target().path(); - auto rel_copy = manifest().relationships(wb_path); + const auto wb_rel_target = manifest().relationship(path("/"), relationship_type::office_document).target(); + auto rel_copy = manifest().relationships(wb_rel_target.path()); // clear existing relations for (const auto &rel : rel_copy) { - manifest().unregister_relationship(uri(wb_path.string()), rel.id()); + manifest().unregister_relationship(wb_rel_target, rel.id()); } // create new relations std::size_t index = 0; From dcf50cb4cd701ce9f797a1275e3913453ac75be3 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 14:17:04 +1200 Subject: [PATCH 03/38] return by const reference when possible --- include/xlnt/packaging/relationship.hpp | 4 ++-- include/xlnt/packaging/uri.hpp | 2 +- source/packaging/relationship.cpp | 4 ++-- source/packaging/uri.cpp | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/include/xlnt/packaging/relationship.hpp b/include/xlnt/packaging/relationship.hpp index 00d7e942..09e88ff2 100644 --- a/include/xlnt/packaging/relationship.hpp +++ b/include/xlnt/packaging/relationship.hpp @@ -130,12 +130,12 @@ public: /// /// Returns the URI of the package part this relationship points to. /// - uri source() const; + const uri &source() const; /// /// Returns the URI of the package part this relationship points to. /// - uri target() const; + const uri &target() const; /// /// Returns true if and only if rhs is equal to this relationship. diff --git a/include/xlnt/packaging/uri.hpp b/include/xlnt/packaging/uri.hpp index 7e945168..66d6358e 100644 --- a/include/xlnt/packaging/uri.hpp +++ b/include/xlnt/packaging/uri.hpp @@ -123,7 +123,7 @@ public: /// Returns the path of this URI. /// E.g. the path of http://example.com/document is "/document" /// - class path path() const; + const class path& path() const; /// /// Returns true if this URI has a non-null query string section. diff --git a/source/packaging/relationship.cpp b/source/packaging/relationship.cpp index 7788be5a..07f7bafa 100644 --- a/source/packaging/relationship.cpp +++ b/source/packaging/relationship.cpp @@ -46,12 +46,12 @@ target_mode relationship::target_mode() const return mode_; } -uri relationship::source() const +const uri &relationship::source() const { return source_; } -uri relationship::target() const +const uri &relationship::target() const { return target_; } diff --git a/source/packaging/uri.cpp b/source/packaging/uri.cpp index 10b43777..850234a3 100644 --- a/source/packaging/uri.cpp +++ b/source/packaging/uri.cpp @@ -16,7 +16,7 @@ std::string uri::to_string() const return path_.string(); } -path uri::path() const +const path& uri::path() const { return path_; } From 53c193433f15726091da43d27cb100fd0605c9be Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 14:18:07 +1200 Subject: [PATCH 04/38] Ensure worksheets get a unique internal filename Issue #279 --- source/workbook/workbook.cpp | 24 ++++++++++++--- .../data/Issue279_workbook_delete_rename.xlsx | Bin 0 -> 11341 bytes tests/workbook/workbook_test_suite.cpp | 29 ++++++++++++++++-- 3 files changed, 46 insertions(+), 7 deletions(-) create mode 100644 tests/data/Issue279_workbook_delete_rename.xlsx diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 4ac1a4d2..01b35e2e 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -738,22 +738,36 @@ worksheet workbook::create_sheet() std::string title = "Sheet1"; int index = 1; + // make a unique sheet name. Sheet<1...n> while (contains(title)) { title = "Sheet" + std::to_string(++index); } - + // unique sheet id size_t sheet_id = 1; for (const auto ws : *this) { sheet_id = std::max(sheet_id, ws.id() + 1); } - std::string sheet_filename = "sheet" + std::to_string(sheet_id) + ".xml"; - d_->worksheets_.push_back(detail::worksheet_impl(this, sheet_id, title)); - + // unique sheet file name auto workbook_rel = d_->manifest_.relationship(path("/"), relationship_type::office_document); - uri relative_sheet_uri(path("worksheets").append(sheet_filename).string()); + auto workbook_files = d_->manifest_.relationships(workbook_rel.target().path()); + auto rel_vec_contains = [&workbook_files](const xlnt::path &new_file_id) { + return workbook_files.end() != std::find_if(workbook_files.begin(), workbook_files.end(), [&new_file_id](const xlnt::relationship &rel) + { + return rel.target().path() == new_file_id; + }); + }; + + size_t file_id = sheet_id; + xlnt::path sheet_relative_path; + do + { + sheet_relative_path = path("worksheets").append("sheet" + std::to_string(file_id++) + ".xml"); + } while (rel_vec_contains(sheet_relative_path)); + + uri relative_sheet_uri(sheet_relative_path.string()); auto absolute_sheet_path = path("/xl").append(relative_sheet_uri.path()); d_->manifest_.register_override_type( absolute_sheet_path, "application/vnd.openxmlformats-officedocument.spreadsheetml.worksheet+xml"); diff --git a/tests/data/Issue279_workbook_delete_rename.xlsx b/tests/data/Issue279_workbook_delete_rename.xlsx new file mode 100644 index 0000000000000000000000000000000000000000..981af413c7412d4facf2aed93913c5688b218a39 GIT binary patch literal 11341 zcmeHtbzBwQ8t$fX)7^-4horQ$gmi~=cSwhz0@B?A(%l`hX{4p4rKG#-ZshaRbcOaBJ<6s}tLCuwle=MttMsq3 z;?0p*@aBbz=cL~4U|+ecEcdIS%@!V*E73-uX7ym&sd=e2K?G{QPeQEhougxXHUuY! zj{wO3)DG7sxri4~%Ytc7$0``Ooi?vcp*6LvtH*o7wTL1_bWX6RecGK$hKn>D1w2Zl z9f%B4@TzC_cCdcGg>80ep2(UPUDkAcMTuwRfEa3NSTz5+tCWm?fcxuGBN;y<_g8~a zw1;uG%YH7DZ5wW9Ow-+45Tp!JEq?L@2Ki{S;*3gwYw2|2gD#FeP4_sIVsGF8nN1yF zTjr`Tx}N)l9{N?^&cK97tToEaH~bB&tfw}5y~b;@%>cQ7p{q)nnQRM8lQbj%fB@dV zus5=FU}C)c4vFl)qsixvrsc~GYYF)DtOk<}6FOdZOe%+;D+-~2`3P=W@hqxe+b z=v@$gee@NdJ{yrNCc^&Kfh?Kchv?LFQCZ`(SUc6&8eA{dG!5plVRLwKcc^@RSDm)l z>~$o80N13+ELMi_wf2GX*|YFX;f+JK{Olq7&$=DFE4CsB&E2Lghhh!KV&1Yr7*yra z@FCF6TZb`r&rBP1zTILmuzFdpq6v^AmXNm9ATU~|T|obzS3zdT*nuc4hUe1mK=;SN*MooUfMG#YO zczIDr7U52W7sNi=k0^3!8=Jf^Q@0%k0BZiZ2CeeE)H>(mon zu~_zz`maMsC;7Ln75gNy zNW!Lv_&g_2Pwm7&92c7kbui1C{Wc%e+vECeqS}T@_@0ftyshQ~eY}mig8WzBW0dHu(@@8OTZ*)hTRfMKR8wu{)UARjW&}DmkXt6|OE4Leq3T|PBf%AtK80x%f zFU@YREys;>bT%%}&dxOkPV=SsTU^Z?-$=w}K(lcQZDb2EZ5nY42ck$LjN>6;ar=lL zUR>la$J#F~^xz>dO6)yjGn1_=k|XfV!gZhX)#nd-9lAKq)gh!+53AWZuMSn)eMqB% z8n63mF@w1(11%-mH%zj@VU`4yP#6yAxEF5-?Og$pe1L<3*n^_bV_RyA#^+DgbZQTM z**tjBOp!6_2TYI1>OER$rF} z`|t1a0Q{eeCDVQ9@S_?$piygG8avI95gY7p8dv}T|JQ2HHue_!HZ~S_e*fN|G>!I{ zgWnq11qTFAIM!2*>WZokNye;jizI%Z(k_HDii`wCuS+0KB(6rZ-~O{c`0ESY*Pmyd zr=e3#^Efnw2*ps-2BdX167Le+5ndpd1agYX zo19sehhekd+6C!4V{50CQzq0B~M}XRAGGL)t>YXH04)DlM)*ydtw773+d}7J_ z-q2X~>wY_{yT`5xUxmqbk}Mmm=?X+q_K{o`{i==StgM*HNbdGNea?{2Kiz*f+!iBT2Zn*& zPmcuvQ2p89Hvkq!eKYIrXg;YnIn1~7Y+g!4N~l_dBFUMeQcGRgS$P^}(xw`AcGMg~ zf>Dv#m<+(LA5pynG7QAMeJY~PptO!;to9w`R0lBz_;y`)`;p?vyL^H?q4(rX2ua%6 z=*JlwLy_0TLPMQA{6FU)&wvu;*088?TUP?GOB6!%Ik64qLh=Y%hnkx%ZK!wc3zJfN z#O7}HaQQxJzkfwOb%QgfXyR@~$Bjrg4 ziyyhYnoy!nWV%6uYNW;IBL^jfLiTZ*Rr&%pY@RV1XZlskCON?=2dErkngRs}$Z0;i z@L^rp;-xn+L&tn-#$`%<%TmZO*+rRQh#oCB0Sw9V`XE*HPhUmJ|(T5=CdkM;<;`I5l@{U@*p^RJx7Og+^p z0dqMG-tGb`a3gRq)w4G;RCENl1rvw6H;O&A34A!Wy1*1tUcPB1RdM!?1BQ^h&}qGi5evq zzNrYPh{zbJbH^*K35cvyN$Mo>Voa>Y#w}NM8i^#}9Qa9#B$8TBzx3~cgB@9G)o=!3 zBA*K7*t}Hnt=QnV&O2cFrgW3UPc^EEmX5oRr_K_9i9T~fOrKwI1}c5;VDqmJNTI2M zMhOQ1m^=jlkp6l=M^{Uu?@!jRr5*N_1Jko)^cGcggDh&dWSF_&6<$4^RSn&?(ssc! z#Hi=!8SPVVRIYccFeEG~eJY?Eg&Lrj+;{3uwdToTS9fN)Wui-%ne?MWl$XR*vEtuQ zzpGeLyIykTI8e@NAqf)neaxl8St>U_dwVsa{fhI!)dA!q6?hSq;4z$F)tW?=*G$YX z9Y`9W;Km9;S0J-%0#lWKj4&$JD76fm;)FH>j9vCPe2p13WTfv^l~#yQbTJ@w%?^D@ z=1mr@naibKLONj|hLDrhB*~1PD$`s`j$M2PB*PxktC1mnH2*z@UO@ylUq|m6K|IE2 z)EGRrS7JXyxH*{O4fz$ex4a5Ukuon4{mG%Zj&LuORB1gwJEV7xE+ z`!yCKEg!b$9LJ^4;Apd_UCpHRGyF8R7wf%N@t1nNmwLaP#-OV%R5If~4VKI{j0D0X5eJ5jPZ%sn%Y|;V0rJV@{T+Tr zdB@$?M0M{#%!9_uZ_4?+<_h6o1#8)kag8-%RKz`*#LXtNmP;cKr4Cw&E=tL}WJ}^# zW)5CYNP`}40DK!Ek4Hx4PJsmtrBQk!B3>*Eb+x29LB55YEhcxj<%Q{T#9HUnL4|H! zyl8$n8LKy~9Jw~VZifwG5AMw8QA?bAYzyz#asV|!F5)=tTs+W13eDTKPejISbud_q z?6F^k%^ z!h--ImG#B`VZj*%@{6&ihOrC$hW9QsC^E=a8sgO^oA0;di=Sj}Vv5-%5bw|CrItEV zwo3EWT6`GJ(|qrXJvKv2{uJtQ1=)5N{o8=D*{7Hx5oM@B5iF>|;C;xx2$pDWGL`N8 zpylf3#p#$7=fb|_`%2fO!6-^1JU+7#&T+;-N=nb8Jc3T4LTg+oP`w_#h{5t^Hb$~V zZq6aYjCT#(avxeN`MD72MuRN}CCM7T7)Meiy8_cY;G0g4kTbHg|8iypU!mOw&KEth z!?XxUl>)=VJn1#&ROQsd>Wm|$iRMHlmF-P`avfuuK3u=SC#}rwV_jY1=W1|gxO`{H zOY{5-^No?-Yd+pm<=*PY3)LRdcKpJR_cUN*31Pi@TVmTtO!2trp)RTY9C-2FHa{C_ zbH_QD5!$+fs8On3)O6YN1OY~D_+ovW>*7(WCYM$`>ag3ocPc!tmOM~5x9D#LGzQsc zpkDhDzaAiMIhqmVCG%TNGtxamT~WY9e1?hp2=Uq4i%`EzR%W_4nTpI(pa_vy%B|NB zTrQDZ%$OeMn1)UgVhK_;45GTA#StL(D&xR|jb-NI|atGHZe6mA~R)=I~_w7?3F zg<#<7r2}i>kWJlZhij%!60Y#V=rv_+`HVuJxIlK*bSI_s%#u{c3$-@~yDan${>mw> zxbzJIv17Wfys`5Sd*}3&dviKS*Ed5}*wm22R!+L?3F!GxLv?qy8Hr?42IGbw7K{zA zmL=b)6@~JLVI6guJF%V5XkM3T5NQ+lZibD0Gd-+xhIZI}xzRn5PoY*|ofGO&q~Tk& z@%;qD{fX34n9JAGd1UicmLUE`@Gom~XnogS;wyA>n1yGa+h-az&=#IA*qc8wfN19@ zf=X;OOPfz)zBasLm{dM-r%ct>WNyGf-9|n0Wr0zA4D(El(nseZo6zBkJ}|cT2tB;xDk5T<{L!N^D;j(6{vY7BoL&>M|BWyV8z5+q*TI>z2AhZX$jQNVJ(3i z3t{aP!DseMd`Z6Brx^5h$oSDsj8yUcvPX}bKns`e=B#}5gguy!eQC~`+NJT4=JIGJ z``#NQSBkh0`=)H+Epj*-lpsCOzj|;GtU^{ZcM7}|6xqH;tTPH2-!77lahCR0dhu~O zlnzbH=P_uoKZ*R^>RHW3gNHHero$E1;&$Q58QC>|sQmSgkL``x!iGA{p-bV-CELDl zS5Cr{gX2Jfa)xigkR}IQ_FL=A`*zshviYxNt-P1KJDIKuK8gtL)FWP2OLP}{5NyR> z#l5sFe9UBip?m|seQ`?*UzBE}>iuZ(#{0*B`p>ZOuUOsD)X2){w_x#7Kg;S93;>Wr zaIejP1B?4=-@jSz;)-o`nXqXQ+-9=k}^b z^G|-^>13qjEAkWejN|koq;c+S2MyyX!+SzqLJEuBZNcK17X5*h#aoS+14fNSkm%^p z2N5%!+H`WXbJbSstsJp9uF|BnoQtIGz}VIrzE@#SZMwJIipHyjrO6OPxuC0Rs4zCr z*(8-Tco0^gV7*BxqLAVOg*lBzDHIAhog$f@#CcKb)RB&n~f5)px*+OMM@um~#RWPwxRZV*)7HCtvz*F_|(7eglCiZMR|GKH* z+N;6WCg$t}=;dNoFkARlQLe?5P5UX;IsIcrLCR!p&Q>14EUK@W3S+JGRf|OwfSbZ z8_XuRo8{%jiYR}9WL`JNgFSb@1t-Gu`i0&Sf%EHLlwlj$k8bB@tgJd4XQ>}AzvVm> zV26r9_Sip?0^)rpj8EyRiDC7#b0J*F;OScU2Gqk4GI%)A8D~?}my>!r zq`Qc8Jq*9@FlD`D4oZ*AduAj+g2j4@XGm~KP70(rJ&Q*5o8*(EvzyH*UWk;%tQVI@ zQ8bVcb1du=nlOWRO-Mp~t)!}7;f!<4#hN!yA+Ai$25n3k^IEyp1!)iyIRtwte7!qK z4Th2oWv#UcT8p;3QWjSnDonyGvW{UD%fUuBUZk^fx6D;d6eyZQxh&IH7do>t;^ose zY04S`Np$%>StwAyaxi8XUS~Vp5XrS=_>1FwR%YZ*#qTiKEhw*7LJ(1O3FlzDfw^-!01p99Khjf67wxU z$ps=q5kiGTJwAJ^x-Et2@W2LVTw}He$t57yvZTtT1e#67eWtgX;NsFaol^T;fy$g5 z$U$MD<;MD$3d*{BwX)B$NO8)NA#sQkp6Vkd*S4D&(i>mA{m5DqAhy|)Q_LQKJNaOQ zfn&RwC2sY=l>EqKHNH(}^|MIF`c?e;aWh8( z1O&!5+@l^tCB0Q~?Ox?NIunk=^0w7-M{^6JV!3vqd~JF^Y1*b3gtFYqk8|dfc3cnP z5JyZ|>F|lxY${uIo--s!2NaBNMfYA2r_zujL+#>*Oi2^!mNq0P$B9$$(ryI3@jlrN$axD^5 zo#P@d3A3bmrG;HOB(fqkaxM?1t*t~|3nR2uMEq;D1G1O;bWdp4RIV3`=L@h6|7Z{XVT~=h-4c3z9Hvu{rC3}YTpy_2~x=#n*Vgs>9$WcVzX+GC*N`W)j zLb(M|rLK}vpE-rXQOH+NC&+YHrkC&mtLOS&Pk4-u_Y+TO5(tUTG%bR_CB#^qEy?oET9CE`(2Wy{|HlB_feye@kzkP**5wR3dL_r_g{2UkAw zMQ4P`JcGs`%yzDM#xQ10wt_qbE0j~qoSo?MegH@v=oO9y?;a_ojBb$i#jRdJFqaV^ z-P6=^YEVKhy-?gHa-HMf=MW2u>N}x;A7Pho2`pRM^i{^6p0bWiwo`M86Uxk=duAps zU?0piyqfSBimgMK&Ik$TK~>4BduI`@zgIw0wWjx&>>KK3!@>T~$dd$OyT&FYvC z(I(=}au~*$!*5^m^~Gs`c9=NayAoEiI6R)d+!xoo;9z~-0}NY-QR_~JLPEL9rV;O81)j|D#-qUesA&J{CiyR zUBN4g;FAR_cyFD_Z~UBiUPiuR8e4~yAW}=a0!-AAs}M~eXvL`IjY{}2C8E(t7cU;80kP8Kmo~6UGRm|0S4j}t4-_rAkz7aENvi!{d+T*I zJ7cGQNTEXXSbQxx1=D#I+LuLwRoMeo4ofrWyyn2*%;-~3uYQ}etR^wlPzWdeMYY7i5yI2&2s>dqq15q`th!^UVJ=q#g_+g1JYH*KaJE_Z$v?6G9< zdxZNBul@8_NR*7tG&ADcU8s|X(6a;`>J1!kw?CN5(fdQR)rVD(j#Q&Q8zrYOJJ}Sc zb3#qS9rDxrO#X16VrAo|1HalImAyU@r{amp0%rA>WA@wgrS*-_iug>6tfz470z%8D z{aq+y5N#aDfIu`*Mf6ylp>u;h*5Qa%Xg=j(NR~4lSJYJ;H zi+qS%+(o)?%uPU9800`K|{=yXOBAi(nf}>csRyTv?&W4`ApC*Tk8wPHBH!8O=xs>*avn-<<#~#tKu|r(!FZeDuUk3YiEzoQfGsHlg|NvH zrmAJ^KGmj*=eAACI<&%J>am!~JpI74T4EbkQHaMCiUF(jo9EX;Vd*Hc=QAb}JcEsy zBkzx2)C))rFq@($*_ik9y~WIYjzh#~@4t4NfBs7!d-q0pf!?hsf!l^7crESUw2jAi zF?L9#yi^-A=9_t#TjB1@L~CxRtZmdzTNK}bi}%@_R%(%&k$qOTb8HTzTShjgLs2Db zGnrb%!Ibk0jY68kX0eKpD!K)hs>6d}$IF4}4{NRJ#1OFfd~LOr2bDGMZ)Yam#BcZ$C%_?M!+Lr>&-6r+aZo=B}ypz!!_NXjJ*yYA@ zcgTe4aprDZx`9!e@OLRYjQ5n*uI$2cL374onX*bq)2S=e%Ep{tuX7{T~zmB}}mWkBR>h zCfNVS#D57B9RFkD{|6JR%DGG0;1T=vzaOzV@0pObfMh}K*nQ*WAKoBlWmQls`81Ol zn031|wY6o<+(X}^c0BF;vPfa?tCbGr;ZoBxB?4|U3sRzlFY9}`vrg;HWa9fkV+egRlo^$dk<5lIQG7@MKSRUfCPQzKrGW7i zXExl~oDG+ky2L76yp>I-t);vYHrBM77?S7_S~BHHs`+##Mt_optxi{e^*Y`W5}Exb zJrQ><=A86|a!am5SKVowyVdip=UNL*>}VLVqb4({!@YeN?DEoygTUSgU#f!OjVM$7 z75YtPJ8Tddj!LjI4^eD&;elRcG+$=d0+OohRx_URdGu z{+@qV*?(R!`B$@_n)|N+f6{910Z72RIGpd-{guq0#{azOJJI02-0>)d_qXxi62cD( z|7m+CBHWidTjzh+{*V-YAlyIu@1nvT!aZMk{|4dbl=-)2c@OyeeETQhJUH73%)u`z z$4{uAPPs?c|K}(PkN$}AJ+}ML wQOLlO&>y_}Judkj<@Ztar+16U|A_K^OqG*@0jG=s065^!DY&Z=q`W)&Ke9O data(std::istream_iterator{file_reader3}, - std::istream_iterator()); + std::vector data(std::istream_iterator{file_reader3}, + std::istream_iterator()); xlnt::workbook wb_load5; wb_load5.load(data); xlnt_assert_equals(wb_path, wb_load5); } + + void test_Issue279() + { + xlnt::workbook wb(path_helper::test_file("Issue279_workbook_delete_rename.xlsx")); + while (wb.sheet_count() > 1) + { + if (wb[1].title() != "BOM") + { + wb.remove_sheet(wb[1]); + } + else + { + wb.remove_sheet(wb[0]); + } + } + // get sheet bom change title + auto ws1 = wb.sheet_by_index(0); + ws1.title("checkedBom"); + // report sheet + auto ws2 = wb.create_sheet(1); + ws2.title("REPORT"); + //save a copy file + wb.save("temp.xlsx"); + } }; static workbook_test_suite x; \ No newline at end of file From ec02121c15dd31cf9d59f6061a135a3222cf87e2 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 15:24:03 +1200 Subject: [PATCH 05/38] Add debug output for a relationship mismatch --- tests/helpers/xml_helper.hpp | 109 ++++++++++++++++++++++------------- 1 file changed, 69 insertions(+), 40 deletions(-) diff --git a/tests/helpers/xml_helper.hpp b/tests/helpers/xml_helper.hpp index 9c051536..9a90e9e3 100644 --- a/tests/helpers/xml_helper.hpp +++ b/tests/helpers/xml_helper.hpp @@ -3,17 +3,17 @@ #include #include +#include +#include #include #include #include -#include -#include class xml_helper { public: static bool compare_files(const std::string &left, - const std::string &right, const std::string &content_type) + const std::string &right, const std::string &content_type) { // content types are stored in unordered maps, too complicated to compare if (content_type == "[Content_Types].xml") @@ -34,7 +34,7 @@ public: } auto is_xml = (content_type.substr(0, 12) == "application/" - && content_type.substr(content_type.size() - 4) == "+xml") + && content_type.substr(content_type.size() - 4) == "+xml") || content_type == "application/xml" || content_type == "[Content_Types].xml" || content_type == "application/vnd.openxmlformats-officedocument.vmlDrawing"; @@ -63,8 +63,7 @@ public: bool difference = false; auto right_iter = right_parser.begin(); - auto is_whitespace = [](const std::string &v) - { + auto is_whitespace = [](const std::string &v) { return v.find_first_not_of("\n\r\t ") == std::string::npos; }; @@ -198,26 +197,26 @@ public: ++right_iter; } - if (difference && !suppress_debug_info) - { - std::cout << "documents don't match" << std::endl; + if (difference && !suppress_debug_info) + { + std::cout << "documents don't match" << std::endl; - std::cout << "left:" << std::endl; + std::cout << "left:" << std::endl; for (auto c : left) { std::cout << c << std::flush; } - std::cout << std::endl; + std::cout << std::endl; - std::cout << "right:" << std::endl; + std::cout << "right:" << std::endl; for (auto c : right) { std::cout << c << std::flush; } - std::cout << std::endl; - } + std::cout << std::endl; + } - return !difference; + return !difference; } static bool compare_relationships(const xlnt::manifest &left, @@ -271,27 +270,26 @@ public: return true; } - static bool xlsx_archives_match(const std::vector &left, + static bool xlsx_archives_match(const std::vector &left, const std::vector &right) - { + { xlnt::detail::vector_istreambuf left_buffer(left); std::istream left_stream(&left_buffer); xlnt::detail::izstream left_archive(left_stream); - const auto left_info = left_archive.files(); + const auto left_info = left_archive.files(); xlnt::detail::vector_istreambuf right_buffer(right); std::istream right_stream(&right_buffer); xlnt::detail::izstream right_archive(right_stream); - const auto right_info = right_archive.files(); + const auto right_info = right_archive.files(); auto difference_is_missing_calc_chain = false; if (std::abs(int(left_info.size()) - int(right_info.size())) == 1) { - auto is_calc_chain = [](const xlnt::path &p) - { + auto is_calc_chain = [](const xlnt::path &p) { return p.filename() == "calcChain.xml"; }; @@ -306,7 +304,7 @@ public: } } - if (left_info.size() != right_info.size() && ! difference_is_missing_calc_chain) + if (left_info.size() != right_info.size() && !difference_is_missing_calc_chain) { std::cout << "left has a different number of files than right" << std::endl; @@ -338,12 +336,41 @@ public: if (!compare_relationships(left_manifest, right_manifest)) { + std::cout << "relationship mismatch\n" + << "Left:\n"; + for (const auto &part : left_manifest.parts()) + { + std::cout << "-part: " << part.string() << '\n'; + auto rels = left_manifest.relationships(part); + for (auto &rel : rels) + { + std::cout << rel.id() << ':' + << static_cast(rel.type()) + << ':' << static_cast(rel.target_mode()) + << ':' << rel.source().path().string() + << ':' << rel.target().path().string() << '\n'; + } + } + std::cout << "\nRight:\n"; + for (const auto &part : right_manifest.parts()) + { + std::cout << "-part: " << part.string() << '\n'; + auto rels = right_manifest.relationships(part); + for (auto &rel : rels) + { + std::cout << rel.id() + << ':' << static_cast(rel.type()) + << ':' << static_cast(rel.target_mode()) + << ':' << rel.source().path().string() + << ':' << rel.target().path().string() << '\n'; + } + } return false; } - for (auto left_member : left_info) - { - if (!right_archive.has_file(left_member)) + for (auto left_member : left_info) + { + if (!right_archive.has_file(left_member)) { if (difference_is_missing_calc_chain) { @@ -357,32 +384,34 @@ public: } auto left_content_type = left_member.string() == "[Content_Types].xml" - ? "[Content_Types].xml" : left_manifest.content_type(left_member); + ? "[Content_Types].xml" + : left_manifest.content_type(left_member); auto right_content_type = left_member.string() == "[Content_Types].xml" - ? "[Content_Types].xml" : right_manifest.content_type(left_member); + ? "[Content_Types].xml" + : right_manifest.content_type(left_member); if (left_content_type != right_content_type) { std::cout << "content types differ: " - << left_member.string() - << " " - << left_content_type - << " " - << right_content_type - << std::endl; + << left_member.string() + << " " + << left_content_type + << " " + << right_content_type + << std::endl; match = false; break; } if (!compare_files(left_archive.read(left_member), - right_archive.read(left_member), left_content_type)) - { - std::cout << left_member.string() << std::endl; + right_archive.read(left_member), left_content_type)) + { + std::cout << left_member.string() << std::endl; match = false; break; - } - } + } + } - return match; - } + return match; + } }; From 7458426111881fa166a9c138e40917e2c0e1a55f Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 15:54:28 +1200 Subject: [PATCH 06/38] return by const ref --- include/xlnt/packaging/relationship.hpp | 2 +- source/packaging/relationship.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/xlnt/packaging/relationship.hpp b/include/xlnt/packaging/relationship.hpp index 09e88ff2..2dcbb4af 100644 --- a/include/xlnt/packaging/relationship.hpp +++ b/include/xlnt/packaging/relationship.hpp @@ -115,7 +115,7 @@ public: /// /// Returns a string of the form rId# that identifies the relationship. /// - std::string id() const; + const std::string& id() const; /// /// Returns the type of this relationship. diff --git a/source/packaging/relationship.cpp b/source/packaging/relationship.cpp index 07f7bafa..9c104e08 100644 --- a/source/packaging/relationship.cpp +++ b/source/packaging/relationship.cpp @@ -36,7 +36,7 @@ relationship::relationship( { } -std::string relationship::id() const +const std::string& relationship::id() const { return id_; } From 0d1bca3fd453712aa1c39e2f3824baf3120deced Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 9 Jul 2018 15:57:10 +1200 Subject: [PATCH 07/38] Remove hidden dependency on unordered map extraction order Issue #279 --- source/workbook/workbook.cpp | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/source/workbook/workbook.cpp b/source/workbook/workbook.cpp index 01b35e2e..c4739f03 100644 --- a/source/workbook/workbook.cpp +++ b/source/workbook/workbook.cpp @@ -754,8 +754,7 @@ worksheet workbook::create_sheet() auto workbook_rel = d_->manifest_.relationship(path("/"), relationship_type::office_document); auto workbook_files = d_->manifest_.relationships(workbook_rel.target().path()); auto rel_vec_contains = [&workbook_files](const xlnt::path &new_file_id) { - return workbook_files.end() != std::find_if(workbook_files.begin(), workbook_files.end(), [&new_file_id](const xlnt::relationship &rel) - { + return workbook_files.end() != std::find_if(workbook_files.begin(), workbook_files.end(), [&new_file_id](const xlnt::relationship &rel) { return rel.target().path() == new_file_id; }); }; @@ -1597,6 +1596,20 @@ bool needs_reorder(const std::unordered_map &title_to_ } return !all_match; // if all are as expected, reorder not required }; + +struct rel_id_sorter +{ + // true if lhs < rhs + bool operator()(const xlnt::relationship &lhs, const xlnt::relationship &rhs) + { + // format is rTd + if (lhs.id().size() < rhs.id().size()) // a number with more digits will be larger + { + return true; + } + return lhs.id() < rhs.id(); + } +}; } // namespace void workbook::reorder_relationships() @@ -1612,6 +1625,7 @@ void workbook::reorder_relationships() // copy of existing relations const auto wb_rel_target = manifest().relationship(path("/"), relationship_type::office_document).target(); auto rel_copy = manifest().relationships(wb_rel_target.path()); + std::sort(rel_copy.begin(), rel_copy.end(), rel_id_sorter{}); // clear existing relations for (const auto &rel : rel_copy) { @@ -1632,7 +1646,7 @@ void workbook::reorder_relationships() rel_it->source(), rel_it->target(), rel_it->target_mode())); } // then all the other relations in the same order they started (just new indices) - for (const auto& old_rel : rel_copy) + for (const auto &old_rel : rel_copy) { if (old_rel.type() == relationship_type::worksheet) { From 49c4e725dcd132d3549696ad53da22b8ad08903c Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 10:29:58 +1200 Subject: [PATCH 08/38] All unimplemented functions under cell/ --- include/xlnt/worksheet/range.hpp | 4 ++-- source/cell/cell.cpp | 26 ++++++++++++++++++++++++++ source/cell/rich_text.cpp | 5 +++++ source/worksheet/range.cpp | 2 +- 4 files changed, 34 insertions(+), 3 deletions(-) diff --git a/include/xlnt/worksheet/range.hpp b/include/xlnt/worksheet/range.hpp index 0abebbcf..02b88a13 100644 --- a/include/xlnt/worksheet/range.hpp +++ b/include/xlnt/worksheet/range.hpp @@ -290,11 +290,11 @@ public: /// bool operator!=(const range &comparand) const; -private: + private: /// /// The worksheet this range is within /// - worksheet ws_; + class worksheet ws_; /// /// The reference of this range diff --git a/source/cell/cell.cpp b/source/cell/cell.cpp index 4426a5dc..96c1a021 100644 --- a/source/cell/cell.cpp +++ b/source/cell/cell.cpp @@ -434,6 +434,18 @@ void cell::hyperlink(xlnt::cell target) value(cell_address); } +void cell::hyperlink(xlnt::range target) +{ + // TODO: should this computed value be a method on a cell? + const auto range_address = target.worksheet().title() + "!" + target.reference().to_string(); + + d_->hyperlink_ = detail::hyperlink_impl(); + d_->hyperlink_.get().relationship = xlnt::relationship("", relationship_type::hyperlink, + uri(""), uri(range_address), target_mode::internal); + d_->hyperlink_.get().display = range_address; + value(range_address); +} + void cell::formula(const std::string &formula) { if (formula.empty()) @@ -472,6 +484,15 @@ void cell::clear_formula() } } +std::string cell::error() const +{ + if (d_->type_ != type::error) + { + throw xlnt::exception("called error() when cell type is not error"); + } + return value(); +} + void cell::error(const std::string &error) { if (error.length() == 0 || error[0] != '#') @@ -743,6 +764,11 @@ calendar cell::base_date() const return workbook().base_date(); } +bool operator==(std::nullptr_t, const cell &cell) +{ + return cell.data_type() == cell::type::empty; +} + XLNT_API std::ostream &operator<<(std::ostream &stream, const xlnt::cell &cell) { return stream << cell.to_string(); diff --git a/source/cell/rich_text.cpp b/source/cell/rich_text.cpp index ad5cccbd..5c3ff4fb 100644 --- a/source/cell/rich_text.cpp +++ b/source/cell/rich_text.cpp @@ -65,6 +65,11 @@ std::vector rich_text::runs() const return runs_; } +void rich_text::runs(const std::vector &new_runs) +{ + runs_ = new_runs; +} + void rich_text::add_run(const rich_text_run &t) { runs_.push_back(t); diff --git a/source/worksheet/range.cpp b/source/worksheet/range.cpp index fa8e8eb2..1226cc0b 100644 --- a/source/worksheet/range.cpp +++ b/source/worksheet/range.cpp @@ -31,7 +31,7 @@ namespace xlnt { -range::range(worksheet ws, const range_reference &reference, major_order order, bool skip_null) +range::range(class worksheet ws, const range_reference &reference, major_order order, bool skip_null) : ws_(ws), ref_(reference), order_(order), From 95ca51e5c8c5a86b6814cde790114b2afb1fcd2c Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 10:43:09 +1200 Subject: [PATCH 09/38] unimplemented functions under styles/ --- source/styles/color.cpp | 10 ++++++++++ source/styles/conditional_format.cpp | 15 +++++++++++++++ source/styles/protection.cpp | 20 ++++++++++++++++++++ source/styles/style.cpp | 5 +++++ 4 files changed, 50 insertions(+) diff --git a/source/styles/color.cpp b/source/styles/color.cpp index e9e1c739..3eccd394 100644 --- a/source/styles/color.cpp +++ b/source/styles/color.cpp @@ -57,6 +57,11 @@ std::size_t indexed_color::index() const return index_; } +void indexed_color::index(std::size_t index) +{ + index_ = index; +} + // theme_color implementation theme_color::theme_color(std::size_t index) : index_(index) @@ -68,6 +73,11 @@ std::size_t theme_color::index() const return index_; } +void theme_color::index(std::size_t index) +{ + index_ = index; +} + // rgb_color implementation std::string rgb_color::hex_string() const diff --git a/source/styles/conditional_format.cpp b/source/styles/conditional_format.cpp index 52736b47..9a5852d5 100644 --- a/source/styles/conditional_format.cpp +++ b/source/styles/conditional_format.cpp @@ -81,6 +81,11 @@ bool conditional_format::operator!=(const conditional_format &other) const return !(*this == other); } +bool conditional_format::has_border() const +{ + return d_->border_id.is_set(); +} + xlnt::border conditional_format::border() const { return d_->parent->borders.at(d_->border_id.get()); @@ -92,6 +97,11 @@ conditional_format conditional_format::border(const xlnt::border &new_border) return *this; } +bool conditional_format::has_fill() const +{ + return d_->fill_id.is_set(); +} + xlnt::fill conditional_format::fill() const { return d_->parent->fills.at(d_->fill_id.get()); @@ -103,6 +113,11 @@ conditional_format conditional_format::fill(const xlnt::fill &new_fill) return *this; } +bool conditional_format::has_font() const +{ + return d_->font_id.is_set(); +} + xlnt::font conditional_format::font() const { return d_->parent->fonts.at(d_->font_id.get()); diff --git a/source/styles/protection.cpp b/source/styles/protection.cpp index 96b32785..3d1fd326 100644 --- a/source/styles/protection.cpp +++ b/source/styles/protection.cpp @@ -26,6 +26,26 @@ namespace xlnt { +protection protection::unlocked_and_visible() +{ + return protection(); +} + +protection protection::locked_and_visible() +{ + return protection().locked(true); +} + +protection protection::unlocked_and_hidden() +{ + return protection().hidden(true); +} + +protection protection::locked_and_hidden() +{ + return protection().locked(true).hidden(true); +} + protection::protection() : locked_(false), hidden_(false) { diff --git a/source/styles/style.cpp b/source/styles/style.cpp index d10f6896..10f145b8 100755 --- a/source/styles/style.cpp +++ b/source/styles/style.cpp @@ -66,6 +66,11 @@ std::size_t style::builtin_id() const return d_->builtin_id.get(); } +bool style::builtin() const +{ + d_->builtin_id.is_set(); +} + std::string style::name() const { return d_->name; From 3d9e887d4ac1d71ceb5f3b388ab6259184d77649 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 10:49:01 +1200 Subject: [PATCH 10/38] unimplemented functions under utils/ --- include/xlnt/utils/exceptions.hpp | 2 +- source/utils/variant.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/xlnt/utils/exceptions.hpp b/include/xlnt/utils/exceptions.hpp index bf088480..0618a0fb 100644 --- a/include/xlnt/utils/exceptions.hpp +++ b/include/xlnt/utils/exceptions.hpp @@ -120,7 +120,7 @@ public: /// /// Default constructor. /// - missing_number_format(); + missing_number_format() = default; /// /// Default copy constructor. diff --git a/source/utils/variant.cpp b/source/utils/variant.cpp index 44d349be..5565fe94 100644 --- a/source/utils/variant.cpp +++ b/source/utils/variant.cpp @@ -41,7 +41,7 @@ variant::variant(const char *value) : variant(std::string(value)) { } -variant::variant(std::int32_t value) +variant::variant(int32_t value) : type_(type::i4), i4_value_(value) { From f9b2ca5929c1d6fa76e9f6508c9c2336e46e1c72 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 10:55:32 +1200 Subject: [PATCH 11/38] unimplemented functions under workbook/ --- include/xlnt/workbook/document_security.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/xlnt/workbook/document_security.hpp b/include/xlnt/workbook/document_security.hpp index 7f9cec49..b6d5624b 100644 --- a/include/xlnt/workbook/document_security.hpp +++ b/include/xlnt/workbook/document_security.hpp @@ -67,7 +67,7 @@ public: /// /// Constructs a new document security object with default values. /// - document_security(); + document_security() = default; /// /// If true, the workbook is locked for revisions. From 650bfeb7dd6d84d9396c72a17e6a0f10f6500bc6 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 10:57:51 +1200 Subject: [PATCH 12/38] unimplemented functions under worksheet -- NOTE: ctor was removed because it can't have been in use, and is duplicated by the overload below it. A pair parameter is only optimal in a very limited number of use cases, and then only slightly over the begin/end overload --- include/xlnt/worksheet/range_reference.hpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/include/xlnt/worksheet/range_reference.hpp b/include/xlnt/worksheet/range_reference.hpp index f5956c75..93643f68 100644 --- a/include/xlnt/worksheet/range_reference.hpp +++ b/include/xlnt/worksheet/range_reference.hpp @@ -56,12 +56,7 @@ public: /// top_left:bottom_right. /// explicit range_reference(const char *range_string); - - /// - /// Constructs a range reference from a pair of cell references. - /// - explicit range_reference(const std::pair &reference_pair); - + /// /// Constructs a range reference from cell references indicating top /// left and bottom right coordinates of the range. From 1390d6a76e0162b87d102d04b1f82c403919df78 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 11:09:41 +1200 Subject: [PATCH 13/38] unimplemented functions in range and path --- source/utils/path.cpp | 16 ++++++++++++++++ source/worksheet/range.cpp | 26 ++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/source/utils/path.cpp b/source/utils/path.cpp index e846a6e5..b67ab977 100644 --- a/source/utils/path.cpp +++ b/source/utils/path.cpp @@ -141,6 +141,22 @@ path::path(const std::string &path_string) { } +path::path(const std::string &path_string, char sep) + : internal_(path_string) +{ + char curr_sep = guess_separator(); + if (curr_sep != sep) + { + for (char& c : internal_) // simple find and replace + { + if (c == curr_sep) + { + c = sep; + } + } + } +} + // general attributes bool path::is_relative() const diff --git a/source/worksheet/range.cpp b/source/worksheet/range.cpp index 1226cc0b..7649edc7 100644 --- a/source/worksheet/range.cpp +++ b/source/worksheet/range.cpp @@ -69,6 +69,11 @@ cell_vector range::operator[](std::size_t index) return vector(index); } +const cell_vector range::operator[](std::size_t index) const +{ + return vector(index); +} + const worksheet &range::target_worksheet() const { return ws_; @@ -112,6 +117,22 @@ cell_vector range::vector(std::size_t vector_index) return cell_vector(ws_, cursor, ref_, order_, skip_null_, false); } +const cell_vector range::vector(std::size_t vector_index) const +{ + auto cursor = ref_.top_left(); + + if (order_ == major_order::row) + { + cursor.row(cursor.row() + static_cast(vector_index)); + } + else + { + cursor.column_index(cursor.column_index() + static_cast(vector_index)); + } + + return cell_vector(ws_, cursor, ref_, order_, skip_null_, false); +} + bool range::contains(const cell_reference &ref) { return ref_.top_left().column_index() <= ref.column_index() @@ -188,6 +209,11 @@ cell range::cell(const cell_reference &ref) return (*this)[ref.row() - 1][ref.column().index - 1]; } +const cell range::cell(const cell_reference &ref) const +{ + return (*this)[ref.row() - 1][ref.column().index - 1]; +} + cell_vector range::front() { return *begin(); From 054f509f7a57bccee07dd3db790202a413f5cd35 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 20:57:09 +1200 Subject: [PATCH 14/38] fix missing return statement --- source/styles/style.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/styles/style.cpp b/source/styles/style.cpp index 10f145b8..5ba4db51 100755 --- a/source/styles/style.cpp +++ b/source/styles/style.cpp @@ -68,7 +68,7 @@ std::size_t style::builtin_id() const bool style::builtin() const { - d_->builtin_id.is_set(); + return d_->builtin_id.is_set(); } std::string style::name() const From 03020cc7931d6103de69b90a465832ac7645bc3b Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 2 Jul 2018 21:06:12 +1200 Subject: [PATCH 15/38] improved hyperlink implementation - hyperlinks to cells and ranges are complete - hyperlink::display is now set as well as the cell value (in excel these can be different) -- if a cell is empty, display is equal to value text -- if a cell has a value, display can be just about anything - This version copies excel in that display is completely ignored once value is set - All hyperlink tests are now part of the cell test suite (not the worksheet test suite which the majority were previously located) --- include/xlnt/cell/cell.hpp | 11 +-- include/xlnt/cell/hyperlink.hpp | 14 +++- source/cell/cell.cpp | 68 +++++++++++-------- source/cell/hyperlink.cpp | 39 +++++++++-- .../detail/implementations/hyperlink_impl.hpp | 7 +- tests/cell/cell_test_suite.cpp | 53 ++++++++++++++- tests/workbook/serialization_test_suite.cpp | 2 +- tests/worksheet/worksheet_test_suite.cpp | 45 ------------ 8 files changed, 144 insertions(+), 95 deletions(-) diff --git a/include/xlnt/cell/cell.hpp b/include/xlnt/cell/cell.hpp index f77f6d31..8b85625b 100644 --- a/include/xlnt/cell/cell.hpp +++ b/include/xlnt/cell/cell.hpp @@ -262,26 +262,21 @@ public: /// class hyperlink hyperlink() const; - /// - /// Adds a hyperlink to this cell pointing to the URL of the given value. - /// - void hyperlink(const std::string &url); - /// /// Adds a hyperlink to this cell pointing to the URI of the given value and sets /// the text value of the cell to the given parameter. /// - void hyperlink(const std::string &url, const std::string &display); + void hyperlink(const std::string &url, const std::string &display = ""); /// /// Adds an internal hyperlink to this cell pointing to the given cell. /// - void hyperlink(xlnt::cell target); + void hyperlink(xlnt::cell target, const std::string& display = ""); /// /// Adds an internal hyperlink to this cell pointing to the given range. /// - void hyperlink(xlnt::range target); + void hyperlink(xlnt::range target, const std::string& display = ""); /// /// Returns true if this cell has a hyperlink set. diff --git a/include/xlnt/cell/hyperlink.hpp b/include/xlnt/cell/hyperlink.hpp index 8034098b..e77f8830 100644 --- a/include/xlnt/cell/hyperlink.hpp +++ b/include/xlnt/cell/hyperlink.hpp @@ -44,12 +44,22 @@ class XLNT_API hyperlink public: bool external() const; class relationship relationship() const; + // external target std::string url() const; + // internal target std::string target_range() const; + + bool has_display() const; void display(const std::string &value); - std::string display() const; + const std::string& display() const; + + bool has_tooltip() const; void tooltip(const std::string &value); - std::string tooltip() const; + const std::string& tooltip() const; + + bool has_location() const; + void location(const std::string &value); + const std::string& location() const; private: friend class cell; diff --git a/source/cell/cell.cpp b/source/cell/cell.cpp index 96c1a021..f080107b 100644 --- a/source/cell/cell.cpp +++ b/source/cell/cell.cpp @@ -372,7 +372,7 @@ hyperlink cell::hyperlink() const return xlnt::hyperlink(&d_->hyperlink_.get()); } -void cell::hyperlink(const std::string &url) +void cell::hyperlink(const std::string &url, const std::string &display) { if (url.empty() || std::find(url.begin(), url.end(), ':') == url.end()) { @@ -388,7 +388,8 @@ void cell::hyperlink(const std::string &url) auto relationships = manifest.relationships(ws.path(), relationship_type::hyperlink); auto relation = std::find_if(relationships.cbegin(), relationships.cend(), [&url](xlnt::relationship rel) { return rel.target().path().string() == url; }); - if (relation != relationships.end()) { + if (relation != relationships.end()) + { d_->hyperlink_.get().relationship = *relation; } else @@ -400,29 +401,20 @@ void cell::hyperlink(const std::string &url) target_mode::external); // TODO: make manifest::register_relationship return the created relationship instead of rel id d_->hyperlink_.get().relationship = manifest.relationship(ws.path(), rel_id); - } - - if (!has_value()) // hyperlink on an empty cell sets the value to the hyperlink string + } + // if a value is already present, the display string is ignored + if (has_value()) { - value(url); + d_->hyperlink_.get().display.set(to_string()); + } + else + { + d_->hyperlink_.get().display.set(display.empty() ? url : display); + value(hyperlink().display()); } } -void cell::hyperlink(const std::string &url, const std::string &display) -{ - if (!display.empty()) // if the display string isn't empty use that - { - value(display); - } - else // empty display string sets the value to the link text - { - value(url); - } - hyperlink(url); - d_->hyperlink_.get().display = display; -} - -void cell::hyperlink(xlnt::cell target) +void cell::hyperlink(xlnt::cell target, const std::string& display) { // TODO: should this computed value be a method on a cell? const auto cell_address = target.worksheet().title() + "!" + target.reference().to_string(); @@ -430,11 +422,19 @@ void cell::hyperlink(xlnt::cell target) d_->hyperlink_ = detail::hyperlink_impl(); d_->hyperlink_.get().relationship = xlnt::relationship("", relationship_type::hyperlink, uri(""), uri(cell_address), target_mode::internal); - d_->hyperlink_.get().display = cell_address; - value(cell_address); + // if a value is already present, the display string is ignored + if (has_value()) + { + d_->hyperlink_.get().display.set(to_string()); + } + else + { + d_->hyperlink_.get().display.set(display.empty() ? cell_address : display); + value(hyperlink().display()); + } } -void cell::hyperlink(xlnt::range target) +void cell::hyperlink(xlnt::range target, const std::string &display) { // TODO: should this computed value be a method on a cell? const auto range_address = target.worksheet().title() + "!" + target.reference().to_string(); @@ -442,8 +442,17 @@ void cell::hyperlink(xlnt::range target) d_->hyperlink_ = detail::hyperlink_impl(); d_->hyperlink_.get().relationship = xlnt::relationship("", relationship_type::hyperlink, uri(""), uri(range_address), target_mode::internal); - d_->hyperlink_.get().display = range_address; - value(range_address); + + // if a value is already present, the display string is ignored + if (has_value()) + { + d_->hyperlink_.get().display.set(to_string()); + } + else + { + d_->hyperlink_.get().display.set(display.empty() ? range_address : display); + value(hyperlink().display()); + } } void cell::formula(const std::string &formula) @@ -828,8 +837,11 @@ void cell::value(const std::string &value_string, bool infer_type) void cell::clear_format() { - format().d_->references -= format().d_->references > 0 ? 1 : 0; - d_->format_.clear(); + if (d_->format_.is_set()) + { + format().d_->references -= format().d_->references > 0 ? 1 : 0; + d_->format_.clear(); + } } void cell::clear_style() diff --git a/source/cell/hyperlink.cpp b/source/cell/hyperlink.cpp index 8d28d84e..28539107 100644 --- a/source/cell/hyperlink.cpp +++ b/source/cell/hyperlink.cpp @@ -67,24 +67,49 @@ bool hyperlink::external() const return d_->relationship.target_mode() == target_mode::external; } -void hyperlink::display(const std::string &value) +bool hyperlink::has_display() const { - d_->display = value; + return d_->display.is_set(); } -std::string hyperlink::display() const +void hyperlink::display(const std::string &value) { - return d_->display; + d_->display.set(value); +} + +const std::string& hyperlink::display() const +{ + return d_->display.get(); +} + +bool hyperlink::has_tooltip() const +{ + return d_->tooltip.is_set(); } void hyperlink::tooltip(const std::string &value) { - d_->tooltip = value; + d_->tooltip.set(value); } -std::string hyperlink::tooltip() const +const std::string& hyperlink::tooltip() const { - return d_->tooltip; + return d_->tooltip.get(); +} + +bool hyperlink::has_location() const +{ + return d_->location.is_set(); +} + +void hyperlink::location(const std::string &value) +{ + d_->location.set(value); +} + +const std::string &hyperlink::location() const +{ + return d_->location.get(); } } // namespace xlnt diff --git a/source/detail/implementations/hyperlink_impl.hpp b/source/detail/implementations/hyperlink_impl.hpp index 84172986..274997c3 100644 --- a/source/detail/implementations/hyperlink_impl.hpp +++ b/source/detail/implementations/hyperlink_impl.hpp @@ -26,15 +26,18 @@ #include #include +#include namespace xlnt { namespace detail { +// [serialised] struct hyperlink_impl { xlnt::relationship relationship; - std::string tooltip; - std::string display; + xlnt::optional location; + xlnt::optional tooltip; + xlnt::optional display; }; inline bool operator==(const hyperlink_impl &lhs, const hyperlink_impl &rhs) diff --git a/tests/cell/cell_test_suite.cpp b/tests/cell/cell_test_suite.cpp index a0753a49..8729134e 100644 --- a/tests/cell/cell_test_suite.cpp +++ b/tests/cell/cell_test_suite.cpp @@ -37,11 +37,13 @@ #include #include #include +#include #include #include #include #include + class cell_test_suite : public test_suite { public: @@ -665,13 +667,60 @@ private: { xlnt::workbook wb; auto cell = wb.active_sheet().cell("A1"); + cell.value(123); xlnt_assert(!cell.has_hyperlink()); xlnt_assert_throws(cell.hyperlink(), xlnt::invalid_attribute); xlnt_assert_throws(cell.hyperlink("notaurl"), xlnt::invalid_parameter); xlnt_assert_throws(cell.hyperlink(""), xlnt::invalid_parameter); - cell.hyperlink("http://example.com"); + // link without optional display + const std::string link1("http://example.com"); + cell.hyperlink(link1); xlnt_assert(cell.has_hyperlink()); - xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), "http://example.com"); + xlnt_assert(cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().url(), link1); + xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), link1); + xlnt_assert_equals(cell.hyperlink().display(), link1); + // link with display + const std::string link2("http://example2.com"); + const std::string display_txt("notaurl"); + cell.hyperlink(link2, display_txt); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().url(), link2); + xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), link2); + xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // cell overload without display + const std::string cell_target_str("A2"); + auto cell_target = wb.active_sheet().cell(cell_target_str); + std::string link3 = wb.active_sheet().title() + '!' + cell_target_str; // Sheet1!A2 + cell.hyperlink(cell_target); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link3); + xlnt_assert_equals(cell.hyperlink().display(), link3); + // cell overload with display + cell.hyperlink(cell_target, display_txt); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link3); + xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // range overload without display + const std::string range_target_str("A2:A5"); + xlnt::range range_target(wb.active_sheet(), xlnt::range_reference(range_target_str)); + std::string link4 = wb.active_sheet().title() + '!' + range_target_str; // Sheet1!A2:A5 + cell.hyperlink(range_target); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link4); + xlnt_assert_equals(cell.hyperlink().display(), link4); + // range overload with display + cell.hyperlink(range_target, display_txt); + xlnt_assert(cell.has_hyperlink()); + xlnt_assert(!cell.hyperlink().external()); + xlnt_assert_equals(cell.hyperlink().target_range(), link4); + xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // value not touched + xlnt_assert_equals(cell.value(), 123); } void test_comment() diff --git a/tests/workbook/serialization_test_suite.cpp b/tests/workbook/serialization_test_suite.cpp index 3533159c..744103ee 100644 --- a/tests/workbook/serialization_test_suite.cpp +++ b/tests/workbook/serialization_test_suite.cpp @@ -575,7 +575,7 @@ public: std::vector destination; source_workbook.save(destination); - source_workbook.save("temp.xlsx"); + source_workbook.save("temp" + source.filename()); #ifdef _MSC_VER std::ifstream source_stream(source.wstring(), std::ios::binary); diff --git a/tests/worksheet/worksheet_test_suite.cpp b/tests/worksheet/worksheet_test_suite.cpp index 84fa2693..b25e0d61 100644 --- a/tests/worksheet/worksheet_test_suite.cpp +++ b/tests/worksheet/worksheet_test_suite.cpp @@ -50,7 +50,6 @@ public: register_test(test_remove_named_range_bad); register_test(test_cell_alternate_coordinates); register_test(test_cell_range_name); - register_test(test_hyperlink_value); register_test(test_rows); register_test(test_no_rows); register_test(test_no_cols); @@ -225,50 +224,6 @@ public: xlnt_assert(c_range_coord[0][0] == c_cell); } - void test_hyperlink_value() - { - xlnt::workbook wb; - auto ws = wb.active_sheet(); - std::string test_link = "http://test.com"; - xlnt::cell_reference test_cell("A1"); - ws.cell(test_cell).hyperlink(test_link); - // when a hyperlink is added to an empty cell, the display text becomes == to the link - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link); - xlnt_assert_equals(ws.cell(test_cell).value(), test_link); - // if the display value changes, the hyperlink remains the same - std::string test_string = "test"; - ws.cell(test_cell).value(test_string); - xlnt_assert_equals(test_string, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link); - // changing the link doesn't change the cell value - std::string test_link2 = "http://test-123.com"; - ws.cell(test_cell).hyperlink(test_link2); - xlnt_assert_equals(test_string, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link2); - // and we can edit both value and hyperlink together - std::string test_string3 = "123test"; - std::string test_link3 = "http://123-test.com"; - ws.cell(test_cell).hyperlink(test_link3, test_string3); - xlnt_assert_equals(test_string3, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link3); - // hyperlinks can also be applied to cells with non-string values - int numeric_test_val = 123; - std::string test_link4 = "http://test-numeric.com"; - xlnt::cell_reference numeric_test_cell("B1"); - ws.cell(numeric_test_cell).value(numeric_test_val); - ws.cell(numeric_test_cell).hyperlink(test_link4); - xlnt_assert_equals(ws.cell(numeric_test_cell).hyperlink().url(), test_link4); - xlnt_assert_equals(ws.cell(numeric_test_cell).value(), numeric_test_val); - // and there should be no issues if two cells use the same hyperlink - ws.cell(numeric_test_cell).hyperlink(test_link3); // still in use on 'A1' - // 'A1' - xlnt_assert_equals(test_string3, ws.cell(test_cell).value()); - xlnt_assert_equals(ws.cell(test_cell).hyperlink().url(), test_link3); - // 'B1' - xlnt_assert_equals(ws.cell(numeric_test_cell).hyperlink().url(), test_link3); - xlnt_assert_equals(ws.cell(numeric_test_cell).value(), numeric_test_val); - } - void test_rows() { xlnt::workbook wb; From ddab6551b02969d125fbc43e1aa0add552757721 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:19:56 +1200 Subject: [PATCH 16/38] Add tests for functionality added to implement declared functionality --- include/xlnt/cell/cell.hpp | 9 ++- include/xlnt/cell/rich_text_run.hpp | 2 +- include/xlnt/styles/color.hpp | 24 +++++- include/xlnt/utils/path.hpp | 7 +- source/cell/cell.cpp | 29 +++---- source/detail/implementations/cell_impl.hpp | 15 ++-- source/styles/color.cpp | 24 +++++- source/styles/style.cpp | 5 ++ source/utils/path.cpp | 7 +- source/utils/variant.cpp | 5 +- tests/cell/cell_test_suite.cpp | 77 +++++++++++++++++-- tests/cell/rich_text_test_suite.cpp | 15 ++++ tests/styles/color_test_suite.cpp | 40 +++++----- .../styles/conditional_format_test_suite.cpp | 64 +++++++++++++++ tests/styles/protection_test_suite.cpp | 55 +++++++++++++ tests/styles/style_test_suite.cpp | 66 ++++++++++++++++ tests/utils/path_test_suite.cpp | 1 + tests/utils/variant_tests.cpp | 69 +++++++++++++++++ tests/worksheet/range_test_suite.cpp | 30 ++++++++ 19 files changed, 479 insertions(+), 65 deletions(-) create mode 100644 tests/styles/conditional_format_test_suite.cpp create mode 100644 tests/styles/protection_test_suite.cpp create mode 100644 tests/styles/style_test_suite.cpp create mode 100644 tests/utils/variant_tests.cpp diff --git a/include/xlnt/cell/cell.hpp b/include/xlnt/cell/cell.hpp index 8b85625b..9ff5fb93 100644 --- a/include/xlnt/cell/cell.hpp +++ b/include/xlnt/cell/cell.hpp @@ -608,9 +608,9 @@ public: bool operator==(const cell &comparand) const; /// - /// Returns true if this cell is uninitialized. + /// Returns false if this cell the same cell as comparand (compared by reference). /// - bool operator==(std::nullptr_t) const; + bool operator!=(const cell &comparand) const; private: friend class style; @@ -646,6 +646,11 @@ private: /// XLNT_API bool operator==(std::nullptr_t, const cell &cell); +/// +/// Returns true if this cell is uninitialized. +/// +XLNT_API bool operator==(const cell &cell, std::nullptr_t); + /// /// Convenience function for writing cell to an ostream. /// Uses cell::to_string() internally. diff --git a/include/xlnt/cell/rich_text_run.hpp b/include/xlnt/cell/rich_text_run.hpp index 0988a7e7..dc31fe52 100644 --- a/include/xlnt/cell/rich_text_run.hpp +++ b/include/xlnt/cell/rich_text_run.hpp @@ -34,7 +34,7 @@ namespace xlnt { /// /// Typedef a rich_text_run as a pair of string and optional font. /// -struct rich_text_run +struct XLNT_API rich_text_run { std::string first; optional second; diff --git a/include/xlnt/styles/color.hpp b/include/xlnt/styles/color.hpp index 609ede8a..db35e6ef 100644 --- a/include/xlnt/styles/color.hpp +++ b/include/xlnt/styles/color.hpp @@ -252,19 +252,37 @@ public: /// Returns the internal indexed color representing this color. If this is not an RGB color, /// an invalid_attribute exception will be thrown. /// - rgb_color rgb() const; + const rgb_color& rgb() const; + + /// + /// Returns the internal indexed color representing this color. If this is not an RGB color, + /// an invalid_attribute exception will be thrown. + /// + rgb_color &rgb(); /// /// Returns the internal indexed color representing this color. If this is not an indexed color, /// an invalid_attribute exception will be thrown. /// - indexed_color indexed() const; + const indexed_color& indexed() const; + + /// + /// Returns the internal indexed color representing this color. If this is not an indexed color, + /// an invalid_attribute exception will be thrown. + /// + indexed_color &indexed(); /// /// Returns the internal indexed color representing this color. If this is not a theme color, /// an invalid_attribute exception will be thrown. /// - theme_color theme() const; + const theme_color& theme() const; + + /// + /// Returns the internal indexed color representing this color. If this is not a theme color, + /// an invalid_attribute exception will be thrown. + /// + theme_color& theme(); /// /// Returns the tint of this color. diff --git a/include/xlnt/utils/path.hpp b/include/xlnt/utils/path.hpp index beff0165..ba16526f 100644 --- a/include/xlnt/utils/path.hpp +++ b/include/xlnt/utils/path.hpp @@ -107,7 +107,7 @@ public: /// Create a string representing this path separated by the provided /// separator or the system-default separator if not provided. /// - std::string string() const; + const std::string& string() const; #ifdef _MSC_VER /// @@ -176,6 +176,11 @@ public: /// bool operator==(const path &other) const; + /// + /// Returns true if left path is equal to right path. + /// + bool operator!=(const path &other) const; + private: /// /// Returns the character that separates directories in the path. diff --git a/source/cell/cell.cpp b/source/cell/cell.cpp index f080107b..bc5f558f 100644 --- a/source/cell/cell.cpp +++ b/source/cell/cell.cpp @@ -341,32 +341,18 @@ cell_reference cell::reference() const return {d_->column_, d_->row_}; } -bool cell::operator==(std::nullptr_t) const -{ - return d_ == nullptr; -} - bool cell::operator==(const cell &comparand) const { return d_ == comparand.d_; } -cell &cell::operator=(const cell &rhs) +bool cell::operator!=(const cell &comparand) const { - d_->column_ = rhs.d_->column_; - d_->format_ = rhs.d_->format_; - d_->formula_ = rhs.d_->formula_; - d_->hyperlink_ = rhs.d_->hyperlink_; - d_->is_merged_ = rhs.d_->is_merged_; - d_->parent_ = rhs.d_->parent_; - d_->row_ = rhs.d_->row_; - d_->type_ = rhs.d_->type_; - d_->value_numeric_ = rhs.d_->value_numeric_; - d_->value_text_ = rhs.d_->value_text_; - - return *this; + return d_ != comparand.d_; } +cell &cell::operator=(const cell &rhs) = default; + hyperlink cell::hyperlink() const { return xlnt::hyperlink(&d_->hyperlink_.get()); @@ -437,7 +423,7 @@ void cell::hyperlink(xlnt::cell target, const std::string& display) void cell::hyperlink(xlnt::range target, const std::string &display) { // TODO: should this computed value be a method on a cell? - const auto range_address = target.worksheet().title() + "!" + target.reference().to_string(); + const auto range_address = target.target_worksheet().title() + "!" + target.reference().to_string(); d_->hyperlink_ = detail::hyperlink_impl(); d_->hyperlink_.get().relationship = xlnt::relationship("", relationship_type::hyperlink, @@ -778,6 +764,11 @@ bool operator==(std::nullptr_t, const cell &cell) return cell.data_type() == cell::type::empty; } +bool operator==(const cell &cell, std::nullptr_t) +{ + return nullptr == cell; +} + XLNT_API std::ostream &operator<<(std::ostream &stream, const xlnt::cell &cell) { return stream << cell.to_string(); diff --git a/source/detail/implementations/cell_impl.hpp b/source/detail/implementations/cell_impl.hpp index ed1833ed..64756770 100644 --- a/source/detail/implementations/cell_impl.hpp +++ b/source/detail/implementations/cell_impl.hpp @@ -27,22 +27,26 @@ #include #include -#include #include +#include #include #include +#include #include namespace xlnt { namespace detail { -struct format_impl; struct worksheet_impl; struct cell_impl { cell_impl(); - + cell_impl(const cell_impl &other) = default; + cell_impl(cell_impl &&other) = default; + cell_impl &operator=(const cell_impl &other) = default; + cell_impl &operator=(cell_impl &&other) = default; + cell_type type_; worksheet_impl *parent_; @@ -72,9 +76,8 @@ inline bool operator==(const cell_impl &lhs, const cell_impl &rhs) && lhs.value_numeric_ == rhs.value_numeric_ && lhs.formula_ == rhs.formula_ && lhs.hyperlink_ == rhs.hyperlink_ - // comparing pointers is unlikely to be what is wanted - /*&& lhs.format_ == rhs.format_ - && lhs.comment_ == rhs.comment_*/; + && (lhs.format_.is_set() == rhs.format_.is_set() && (!lhs.format_.is_set() || *lhs.format_.get() == *rhs.format_.get())) + && (lhs.comment_.is_set() == rhs.comment_.is_set() && (!lhs.comment_.is_set() || *lhs.comment_.get() == *rhs.comment_.get())); } } // namespace detail diff --git a/source/styles/color.cpp b/source/styles/color.cpp index 3eccd394..01561eec 100644 --- a/source/styles/color.cpp +++ b/source/styles/color.cpp @@ -233,19 +233,37 @@ void color::auto_(bool value) auto__ = value; } -indexed_color color::indexed() const +const indexed_color& color::indexed() const { assert_type(color_type::indexed); return indexed_; } -theme_color color::theme() const +indexed_color &color::indexed() +{ + assert_type(color_type::indexed); + return indexed_; +} + +const theme_color& color::theme() const { assert_type(color_type::theme); return theme_; } -rgb_color color::rgb() const +theme_color &color::theme() +{ + assert_type(color_type::theme); + return theme_; +} + +const rgb_color& color::rgb() const +{ + assert_type(color_type::rgb); + return rgb_; +} + +rgb_color &color::rgb() { assert_type(color_type::rgb); return rgb_; diff --git a/source/styles/style.cpp b/source/styles/style.cpp index 5ba4db51..4dd5418d 100755 --- a/source/styles/style.cpp +++ b/source/styles/style.cpp @@ -92,6 +92,11 @@ bool style::operator==(const style &other) const return name() == other.name(); } +bool style::operator!=(const style &other) const +{ + return !operator==(other); +} + xlnt::alignment style::alignment() const { return d_->parent->alignments.at(d_->alignment_id.get()); diff --git a/source/utils/path.cpp b/source/utils/path.cpp index b67ab977..34c36cf0 100644 --- a/source/utils/path.cpp +++ b/source/utils/path.cpp @@ -221,7 +221,7 @@ std::pair path::split_extension() const // conversion -std::string path::string() const +const std::string& path::string() const { return internal_; } @@ -347,4 +347,9 @@ bool path::operator==(const path &other) const return internal_ == other.internal_; } +bool path::operator!=(const path &other) const +{ + return !operator==(other); +} + } // namespace xlnt diff --git a/source/utils/variant.cpp b/source/utils/variant.cpp index 5565fe94..4c420eea 100644 --- a/source/utils/variant.cpp +++ b/source/utils/variant.cpp @@ -27,9 +27,8 @@ namespace xlnt { variant::variant() -{ - -} + : type_(type::null) +{} variant::variant(const std::string &value) : type_(type::lpstr), diff --git a/tests/cell/cell_test_suite.cpp b/tests/cell/cell_test_suite.cpp index 8729134e..510786b3 100644 --- a/tests/cell/cell_test_suite.cpp +++ b/tests/cell/cell_test_suite.cpp @@ -29,20 +29,20 @@ #include #include #include -#include #include #include #include #include +#include #include #include -#include -#include #include #include #include #include - +#include +#include +#include class cell_test_suite : public test_suite { @@ -81,6 +81,7 @@ public: register_test(test_anchor); register_test(test_hyperlink); register_test(test_comment); + register_test(test_copy_and_compare); } private: @@ -253,11 +254,27 @@ private: xlnt::workbook wb; auto ws = wb.active_sheet(); auto cell = ws.cell(xlnt::cell_reference(1, 1)); + // error string can't be empty + xlnt_assert_throws(cell.error(""), xlnt::exception); + // error string has to have a leading '#' + xlnt_assert_throws(cell.error("not an error"), xlnt::exception); for (auto error_code : xlnt::cell::error_codes()) { + // error type from the string format cell.value(error_code.first, true); xlnt_assert(cell.data_type() == xlnt::cell::type::error); + std::string error; + xlnt_assert_throws_nothing(error = cell.error()); + xlnt_assert_equals(error, error_code.first); + // clearing the value clears the error state + cell.clear_value(); + xlnt_assert_throws(cell.error(), xlnt::exception); + // can explicitly set the error + xlnt_assert_throws_nothing(cell.error(error_code.first)); + std::string error2; + xlnt_assert_throws_nothing(error2 = cell.error()); + xlnt_assert_equals(error2, error_code.first); } } @@ -667,7 +684,7 @@ private: { xlnt::workbook wb; auto cell = wb.active_sheet().cell("A1"); - cell.value(123); + xlnt_assert(!cell.has_hyperlink()); xlnt_assert_throws(cell.hyperlink(), xlnt::invalid_attribute); xlnt_assert_throws(cell.hyperlink("notaurl"), xlnt::invalid_parameter); @@ -680,7 +697,8 @@ private: xlnt_assert_equals(cell.hyperlink().url(), link1); xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), link1); xlnt_assert_equals(cell.hyperlink().display(), link1); - // link with display + cell.clear_value(); + // link with display const std::string link2("http://example2.com"); const std::string display_txt("notaurl"); cell.hyperlink(link2, display_txt); @@ -689,6 +707,14 @@ private: xlnt_assert_equals(cell.hyperlink().url(), link2); xlnt_assert_equals(cell.hyperlink().relationship().target().to_string(), link2); xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // value + int cell_test_val = 123; + cell.value(cell_test_val); + std::string cell_value_str = std::to_string(cell_test_val); + cell.hyperlink(link2, display_txt); + xlnt_assert_equals(cell.value(), 123); + xlnt_assert_equals(cell.hyperlink().display(), cell_value_str); // display text ignored + cell.clear_value(); // cell overload without display const std::string cell_target_str("A2"); auto cell_target = wb.active_sheet().cell(cell_target_str); @@ -698,12 +724,19 @@ private: xlnt_assert(!cell.hyperlink().external()); xlnt_assert_equals(cell.hyperlink().target_range(), link3); xlnt_assert_equals(cell.hyperlink().display(), link3); + cell.clear_value(); // cell overload with display cell.hyperlink(cell_target, display_txt); xlnt_assert(cell.has_hyperlink()); xlnt_assert(!cell.hyperlink().external()); xlnt_assert_equals(cell.hyperlink().target_range(), link3); xlnt_assert_equals(cell.hyperlink().display(), display_txt); + // value + cell.value(cell_test_val); + cell.hyperlink(cell_target, display_txt); + xlnt_assert_equals(cell.value(), 123); + xlnt_assert_equals(cell.hyperlink().display(), cell_value_str); // display text ignored + cell.clear_value(); // range overload without display const std::string range_target_str("A2:A5"); xlnt::range range_target(wb.active_sheet(), xlnt::range_reference(range_target_str)); @@ -713,14 +746,19 @@ private: xlnt_assert(!cell.hyperlink().external()); xlnt_assert_equals(cell.hyperlink().target_range(), link4); xlnt_assert_equals(cell.hyperlink().display(), link4); + cell.clear_value(); // range overload with display cell.hyperlink(range_target, display_txt); xlnt_assert(cell.has_hyperlink()); xlnt_assert(!cell.hyperlink().external()); xlnt_assert_equals(cell.hyperlink().target_range(), link4); xlnt_assert_equals(cell.hyperlink().display(), display_txt); - // value not touched + // value + cell.value(cell_test_val); + cell.hyperlink(range_target, display_txt); xlnt_assert_equals(cell.value(), 123); + xlnt_assert_equals(cell.hyperlink().display(), cell_value_str); // display text ignored + cell.clear_value(); } void test_comment() @@ -737,6 +775,31 @@ private: xlnt_assert(!cell.has_comment()); xlnt_assert_throws(cell.comment(), xlnt::exception); } + + void test_copy_and_compare() + { + xlnt::workbook wb; + auto ws = wb.active_sheet(); + auto cell1 = ws.cell("A1"); + auto cell2 = ws.cell("A2"); + + xlnt_assert_equals(cell1, cell1); + xlnt_assert_equals(cell2, cell2); + xlnt_assert_differs(cell1, cell2); + xlnt_assert_differs(cell2, cell1); + // nullptr equality + xlnt_assert(nullptr == cell1); + xlnt_assert(cell1 == nullptr); + cell1.value("test"); + xlnt_assert(!(nullptr == cell1)); + xlnt_assert(!(cell1 == nullptr)); + // copy + xlnt::cell cell3(cell1); + xlnt_assert_equals(cell1, cell3); + // assign + cell3 = cell2; + xlnt_assert_equals(cell2, cell3); + } }; static cell_test_suite x{}; \ No newline at end of file diff --git a/tests/cell/rich_text_test_suite.cpp b/tests/cell/rich_text_test_suite.cpp index af44f815..3af26383 100644 --- a/tests/cell/rich_text_test_suite.cpp +++ b/tests/cell/rich_text_test_suite.cpp @@ -35,6 +35,7 @@ public: rich_text_test_suite() { register_test(test_operators); + register_test(test_runs); } void test_operators() @@ -100,5 +101,19 @@ public: text_family_differs.add_run(run_family_differs); xlnt_assert_differs(text_formatted, text_family_differs); } + + void test_runs() + { + xlnt::rich_text rt; + xlnt_assert(rt.runs().empty()); + std::vector test_runs{xlnt::rich_text_run{"1_abc_test_123"}, xlnt::rich_text_run{"2_abc_test_123"}, xlnt::rich_text_run{"3_abc_test_123"}}; + // just one + rt.add_run(test_runs[0]); + xlnt_assert_equals(1, rt.runs().size()); + xlnt_assert_equals(test_runs[0], rt.runs()[0]); + // whole set + rt.runs(test_runs); + xlnt_assert_equals(test_runs, rt.runs()); + } }; static rich_text_test_suite x{}; \ No newline at end of file diff --git a/tests/styles/color_test_suite.cpp b/tests/styles/color_test_suite.cpp index ac61291b..e65f540a 100644 --- a/tests/styles/color_test_suite.cpp +++ b/tests/styles/color_test_suite.cpp @@ -38,37 +38,39 @@ public: void test_known_colors() { - const std::vector> known_colors - { - { xlnt::color::black(), "FF000000" }, - { xlnt::color::white(), "FFFFFFFF" }, - { xlnt::color::red(), "FFFF0000" }, - { xlnt::color::darkred(), "FF8B0000" }, - { xlnt::color::blue(), "FF0000FF" }, - { xlnt::color::darkblue(), "FF00008B" }, - { xlnt::color::green(), "FF00FF00" }, - { xlnt::color::darkgreen(), "FF008B00" }, - { xlnt::color::yellow(), "FFFFFF00" }, - { xlnt::color::darkyellow(), "FFCCCC00" } - }; - + const std::vector> known_colors{ + {xlnt::color::black(), "FF000000"}, + {xlnt::color::white(), "FFFFFFFF"}, + {xlnt::color::red(), "FFFF0000"}, + {xlnt::color::darkred(), "FF8B0000"}, + {xlnt::color::blue(), "FF0000FF"}, + {xlnt::color::darkblue(), "FF00008B"}, + {xlnt::color::green(), "FF00FF00"}, + {xlnt::color::darkgreen(), "FF008B00"}, + {xlnt::color::yellow(), "FFFFFF00"}, + {xlnt::color::darkyellow(), "FFCCCC00"}}; + for (auto pair : known_colors) { xlnt_assert_equals(pair.first.rgb().hex_string(), pair.second); } } - + void test_non_rgb_colors() { - xlnt::color indexed = xlnt::indexed_color(1); - xlnt_assert(!indexed.auto_()); + xlnt::color indexed = xlnt::indexed_color(1); + xlnt_assert(!indexed.auto_()); xlnt_assert_equals(indexed.indexed().index(), 1); + indexed.indexed().index(2); + xlnt_assert_equals(indexed.indexed().index(), 2); xlnt_assert_throws(indexed.theme(), xlnt::invalid_attribute); xlnt_assert_throws(indexed.rgb(), xlnt::invalid_attribute); - xlnt::color theme = xlnt::theme_color(3); - xlnt_assert(!theme.auto_()); + xlnt::color theme = xlnt::theme_color(3); + xlnt_assert(!theme.auto_()); xlnt_assert_equals(theme.theme().index(), 3); + theme.theme().index(4); + xlnt_assert_equals(theme.theme().index(), 4); xlnt_assert_throws(theme.indexed(), xlnt::invalid_attribute); xlnt_assert_throws(theme.rgb(), xlnt::invalid_attribute); } diff --git a/tests/styles/conditional_format_test_suite.cpp b/tests/styles/conditional_format_test_suite.cpp new file mode 100644 index 00000000..1410774f --- /dev/null +++ b/tests/styles/conditional_format_test_suite.cpp @@ -0,0 +1,64 @@ +// Copyright (c) 2014-2018 Thomas Fussell +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE +// +// @license: http://www.opensource.org/licenses/mit-license.php +// @author: see AUTHORS file + +#include +#include +#include + +class conditional_format_test_suite : public test_suite +{ +public: + conditional_format_test_suite() + { + register_test(test_all); + } + + void test_all() + { + xlnt::workbook wb; + auto ws = wb.active_sheet(); + auto format = ws.conditional_format(xlnt::range_reference("A1:A10"), xlnt::condition::text_contains("test")); + xlnt_assert(!format.has_border()); + xlnt_assert(!format.has_fill()); + xlnt_assert(!format.has_font()); + // set border + auto border = xlnt::border().diagonal(xlnt::diagonal_direction::both); + format.border(border); + xlnt_assert(format.has_border()); + xlnt_assert_equals(format.border(), border); + // set fill + auto fill = xlnt::fill(xlnt::gradient_fill().type(xlnt::gradient_fill_type::path)); + format.fill(fill); + xlnt_assert(format.has_fill()); + xlnt_assert_equals(format.fill(), fill); + // set font + auto font = xlnt::font().color(xlnt::color::darkblue()); + format.font(font); + xlnt_assert(format.has_font()); + xlnt_assert_equals(format.font(), font); + // copy ctor + auto format_copy(format); + xlnt_assert_equals(format, format_copy); + } +}; +static conditional_format_test_suite x; \ No newline at end of file diff --git a/tests/styles/protection_test_suite.cpp b/tests/styles/protection_test_suite.cpp new file mode 100644 index 00000000..039d987c --- /dev/null +++ b/tests/styles/protection_test_suite.cpp @@ -0,0 +1,55 @@ +// Copyright (c) 2014-2018 Thomas Fussell +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE +// +// @license: http://www.opensource.org/licenses/mit-license.php +// @author: see AUTHORS file + +#include +#include +#include + +class protection_test_suite : public test_suite +{ +public: + protection_test_suite() + { + register_test(test_all); + } + + void test_all() + { + auto prot = xlnt::protection::unlocked_and_visible(); + xlnt_assert(!prot.hidden()); + xlnt_assert(!prot.locked()); + + prot = xlnt::protection::locked_and_visible(); + xlnt_assert(!prot.hidden()); + xlnt_assert(prot.locked()); + + prot = xlnt::protection::unlocked_and_hidden(); + xlnt_assert(prot.hidden()); + xlnt_assert(!prot.locked()); + + prot = xlnt::protection::locked_and_hidden(); + xlnt_assert(prot.hidden()); + xlnt_assert(prot.locked()); + } +}; +static protection_test_suite x; \ No newline at end of file diff --git a/tests/styles/style_test_suite.cpp b/tests/styles/style_test_suite.cpp new file mode 100644 index 00000000..3cd22a15 --- /dev/null +++ b/tests/styles/style_test_suite.cpp @@ -0,0 +1,66 @@ +// Copyright (c) 2014-2018 Thomas Fussell +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE +// +// @license: http://www.opensource.org/licenses/mit-license.php +// @author: see AUTHORS file + +#include +#include +#include + +class style_test_suite : public test_suite +{ +public: + style_test_suite() + { + register_test(test_all); + } + + void test_all() + { + xlnt::workbook wb; + auto ws = wb.active_sheet(); + auto test_style = wb.create_style("test_style"); + test_style.number_format(xlnt::number_format::date_ddmmyyyy()); + + auto copy_style(test_style); + xlnt_assert_equals(test_style, copy_style); + + // number format + xlnt_assert_equals(copy_style.name(), "test_style"); + xlnt_assert_equals(copy_style.number_format(), xlnt::number_format::date_ddmmyyyy()); + //xlnt_assert(!copy_style.number_format_applied()); // this doesn't seem to have sensible behaviour? + copy_style.number_format(xlnt::number_format::date_datetime(), true); // true applied param + xlnt_assert_equals(copy_style.number_format(), xlnt::number_format::date_datetime()); + xlnt_assert(copy_style.number_format_applied()); + copy_style.number_format(xlnt::number_format::date_dmminus(), false); // false applied param + xlnt_assert_equals(copy_style.number_format(), xlnt::number_format::date_dmminus()); + xlnt_assert(!copy_style.number_format_applied()); + + xlnt_assert(!copy_style.pivot_button()); + copy_style.pivot_button(true); + xlnt_assert(copy_style.pivot_button()); + + xlnt_assert(!copy_style.quote_prefix()); + copy_style.quote_prefix(true); + xlnt_assert(copy_style.quote_prefix()); + } +}; +static style_test_suite x; \ No newline at end of file diff --git a/tests/utils/path_test_suite.cpp b/tests/utils/path_test_suite.cpp index f8d4f24c..bfc6fdfa 100644 --- a/tests/utils/path_test_suite.cpp +++ b/tests/utils/path_test_suite.cpp @@ -26,6 +26,7 @@ #include #include #include +#include class path_test_suite : public test_suite { diff --git a/tests/utils/variant_tests.cpp b/tests/utils/variant_tests.cpp new file mode 100644 index 00000000..10de95ec --- /dev/null +++ b/tests/utils/variant_tests.cpp @@ -0,0 +1,69 @@ +// Copyright (c) 2014-2018 Thomas Fussell +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE +// +// @license: http://www.opensource.org/licenses/mit-license.php +// @author: see AUTHORS file + +#include +#include + +class variant_test_suite : public test_suite +{ +public: + variant_test_suite() + : test_suite() + { + register_test(test_null); + register_test(test_int32); + register_test(test_string); + } + + void test_null() + { + xlnt::variant var_null; + xlnt_assert_equals(var_null.value_type(), xlnt::variant::type::null); + xlnt_assert(var_null.is(xlnt::variant::type::null)); + } + + void test_int32() + { + xlnt::variant var_int(std::int32_t(10)); + xlnt_assert_equals(var_int.value_type(), xlnt::variant::type::i4); + xlnt_assert(var_int.is(xlnt::variant::type::i4)); + xlnt_assert_throws_nothing(var_int.get()); + xlnt_assert_equals(10, var_int.get()); + } + + void test_string() + { + xlnt::variant var_str1("test1"); + xlnt_assert_equals(var_str1.value_type(), xlnt::variant::type::lpstr); + xlnt_assert(var_str1.is(xlnt::variant::type::lpstr)); + xlnt_assert_throws_nothing(var_str1.get()); + xlnt_assert_equals("test1", var_str1.get()); + + xlnt::variant var_str2(std::string("test2")); + xlnt_assert_equals(var_str2.value_type(), xlnt::variant::type::lpstr); + xlnt_assert(var_str2.is(xlnt::variant::type::lpstr)); + xlnt_assert_throws_nothing(var_str2.get()); + xlnt_assert_equals("test2", var_str2.get()); + } +}; +static variant_test_suite x; \ No newline at end of file diff --git a/tests/worksheet/range_test_suite.cpp b/tests/worksheet/range_test_suite.cpp index 98012289..b8c14fb6 100644 --- a/tests/worksheet/range_test_suite.cpp +++ b/tests/worksheet/range_test_suite.cpp @@ -37,10 +37,40 @@ class range_test_suite : public test_suite public: range_test_suite() { + register_test(test_construction); register_test(test_batch_formatting); register_test(test_clear_cells); } + void test_construction() + { + xlnt::workbook wb; + auto ws = wb.active_sheet(); + + xlnt::range range_1(ws, xlnt::range_reference("A1:D10")); + xlnt_assert_equals(range_1.target_worksheet(), ws); + xlnt_assert_equals(1, range_1.front()[0].row()); // NOTE: querying row/column here desperately needs some shortcuts + xlnt_assert_equals(xlnt::column_t("D"), range_1.front().back().column()); + xlnt_assert_equals(10, range_1.back()[0].row()); + xlnt_assert_equals(xlnt::column_t("D"), range_1.back().back().column()); + // assert default parameters in ctor + xlnt::range range_2(ws, xlnt::range_reference("A1:D10"), xlnt::major_order::row, false); + xlnt_assert_equals(range_1, range_2); + // assert copy + xlnt::range range_3(range_2); + xlnt_assert_equals(range_1, range_3); + + // column order + xlnt::range range_4(ws, xlnt::range_reference("A1:D10"), xlnt::major_order::column); + xlnt_assert_equals(xlnt::column_t("A"), range_4.front()[0].column()); // NOTE: querying row/column here desperately needs some shortcuts + xlnt_assert_equals(10, range_4.front().back().row()); + xlnt_assert_equals(xlnt::column_t("D"), range_4.back()[0].column()); + xlnt_assert_equals(10, range_4.back().back().row()); + // assignment + range_3 = range_4; + xlnt_assert_equals(range_3, range_4); + } + void test_batch_formatting() { xlnt::workbook wb; From c94fc5a9997ea058cd27d624a1103afb6430461c Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 14 Jul 2018 17:18:28 +1200 Subject: [PATCH 17/38] Travis CI setup modifications --- .travis.yml | 110 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 91 insertions(+), 19 deletions(-) diff --git a/.travis.yml b/.travis.yml index 3c9e7c85..f89d204d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,34 +1,106 @@ -language: cpp +# cpp takes longer +language: minimal sudo: false dist: trusty +git: + depth: false + notifications: email: false - -env: - - COVERAGE=ON STATIC=ON SAMPLES=OFF BENCHMARKS=OFF BUILD_TYPE=Debug - - COVERAGE=OFF STATIC=ON SAMPLES=ON BENCHMARKS=OFF BUILD_TYPE=Debug - - COVERAGE=OFF STATIC=OFF SAMPLES=ON BENCHMARKS=OFF BUILD_TYPE=Debug - - COVERAGE=OFF STATIC=ON SAMPLES=ON BENCHMARKS=ON BUILD_TYPE=Release - - COVERAGE=OFF STATIC=OFF SAMPLES=ON BENCHMARKS=ON BUILD_TYPE=Release - -addons: - apt: - sources: - - ubuntu-toolchain-r-test - packages: - - g++-6 lcov + +# set up build matrix +matrix: + include: + # gcc-6, c++11, debug build, static linking, code coverage enabled + - os: linux + compiler: gcc + addons: + apt: + sources: + - ubuntu-toolchain-r-test + packages: + - g++-6 + - lcov + env: + - CXX_COMPILER=g++-6 + - C_COMPILER=gcc-6 + - BUILD_TYPE=Debug + - COVERAGE=ON + - STATIC=ON + - SAMPLES=OFF + - BENCHMARKS=OFF + # gcc-6, c++11, debug build, dll + - os: linux + compiler: gcc + addons: + apt: + sources: + - ubuntu-toolchain-r-test + packages: + - g++-6 + env: + - CXX_COMPILER=g++-6 + - C_COMPILER=gcc-6 + - BUILD_TYPE=Debug + - COVERAGE=OFF + - STATIC=OFF + - SAMPLES=OFF + - BENCHMARKS=OFF + # gcc-8, c++11, release build, static linking, samples + benchmarks compiled + - os: linux + compiler: gcc + addons: + apt: + sources: + - ubuntu-toolchain-r-test + packages: + - g++-8 + env: + - CXX_COMPILER=g++-8 + - C_COMPILER=gcc-8 + - BUILD_TYPE=Release + - COVERAGE=OFF + - STATIC=ON + - SAMPLES=ON + - BENCHMARKS=ON + # clang 6, c++11, release build, static linking, samples + benchmarks compiled + - os: linux + compiler: clang + addons: + apt: + sources: + - ubuntu-toolchain-r-test + - llvm-toolchain-trusty-6.0 + packages: + - clang-6.0 + env: + - CXX_COMPILER=clang++-6.0 + - C_COMPILER=clang-6.0 + - BUILD_TYPE=Release + - COVERAGE=OFF + - STATIC=ON + - SAMPLES=ON + - BENCHMARKS=ON + +before_install: + - export CC=${C_COMPILER} + - export CXX=${CXX_COMPILER} install: - - export CC=gcc-6 - - export CXX=g++-6 - - gem install coveralls-lcov + - | + if [[ "${COVERAGE}" == "ON" ]]; then + gem install coveralls-lcov; + fi + + - ${CXX} --version + - cmake --version script: - mkdir build - cd build - cmake -D STATIC=$STATIC -D BENCHMARKS=$BENCHMARKS -D SAMPLES=$SAMPLES -D COVERAGE=$COVERAGE -D CMAKE_BUILD_TYPE=$BUILD_TYPE .. - - cmake --build . + - cmake --build . -- -j2 - ./tests/xlnt.test after_success: From d4cc538faf6989d61a0cc1ac29047cc93b7bff68 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 14 Jul 2018 18:42:57 +1200 Subject: [PATCH 18/38] remove -Werror from clang build to allow CI to run through -- fixing CI warnings 1 at a time would be a complete waste of time -- blacklist some of the overkill warnings --- source/CMakeLists.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index ded96a82..739d2c5a 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -37,12 +37,15 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "GNU") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas") # ignore MSVC and Clang pragmas elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Weverything") # all warnings - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror") # warnings are errors + # blacklist warnings that are not relevant set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-c++98-compat") # ignore warnings about C++98 compatibility set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-c++98-compat-pedantic") # ignore pedantic warnings about C++98 compatibility set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-padded") # ignore padding warnings set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-documentation-unknown-command") # ignore unknown commands in Javadoc-style comments set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas") # ignore Windows and GCC pragmas + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-float-equal") # don't warn on uses of == for fp types + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-newline-eof") # no longer an issue with post-c++11 standards which mandate include add a newline if neccesary + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-covered-switch-default") # default is often added to switches for completeness or to cover future alternatives endif() if(STATIC_CRT) From 9a33210144d5e313926171da929553254c7082e9 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 14 Jul 2018 19:52:21 +1200 Subject: [PATCH 19/38] Resolve warnings about global ctors and an unused variable -- while unlikely to become an issue, ordering of ctors across source files is undefined and debugging issues related to it is not easy so just avoid that issue --- source/cell/rich_text.cpp | 11 ++++- source/detail/serialization/xlsx_producer.cpp | 10 +---- source/styles/font.cpp | 12 ++++-- source/worksheet/phonetic_pr.cpp | 41 +++++++++++-------- 4 files changed, 44 insertions(+), 30 deletions(-) diff --git a/source/cell/rich_text.cpp b/source/cell/rich_text.cpp index ad5cccbd..820bba5d 100644 --- a/source/cell/rich_text.cpp +++ b/source/cell/rich_text.cpp @@ -26,15 +26,22 @@ #include #include +namespace { +bool has_trailing_whitespace(const std::string &s) +{ + return !s.empty() && (s.front() == ' ' || s.back() == ' '); +}; +} + namespace xlnt { rich_text::rich_text(const std::string &plain_text) - : rich_text(rich_text_run{plain_text, optional(), false}) + : rich_text(rich_text_run{plain_text, optional(), has_trailing_whitespace(plain_text)}) { } rich_text::rich_text(const std::string &plain_text, const class font &text_font) - : rich_text(rich_text_run{plain_text, optional(text_font), false}) + : rich_text(rich_text_run{plain_text, optional(text_font), has_trailing_whitespace(plain_text)}) { } diff --git a/source/detail/serialization/xlsx_producer.cpp b/source/detail/serialization/xlsx_producer.cpp index aef2a050..d2c27a92 100644 --- a/source/detail/serialization/xlsx_producer.cpp +++ b/source/detail/serialization/xlsx_producer.cpp @@ -816,7 +816,6 @@ void xlsx_producer::write_pivot_table(const relationship & /*rel*/) void xlsx_producer::write_shared_string_table(const relationship & /*rel*/) { static const auto &xmlns = constants::ns("spreadsheetml"); - static const auto &xmlns_xml = constants::ns("xml"); write_start_element(xmlns, "sst"); write_namespace(xmlns, ""); @@ -853,18 +852,13 @@ void xlsx_producer::write_shared_string_table(const relationship & /*rel*/) write_attribute("count", string_count); write_attribute("uniqueCount", source_.shared_strings().size()); - auto has_trailing_whitespace = [](const std::string &s) - { - return !s.empty() && (s.front() == ' ' || s.back() == ' '); - }; - for (const auto &string : source_.shared_strings()) { if (string.runs().size() == 1 && !string.runs().at(0).second.is_set()) { write_start_element(xmlns, "si"); write_start_element(xmlns, "t"); - write_characters(string.plain_text(),string.runs().front().preserve_space); + write_characters(string.plain_text(), string.runs().front().preserve_space); write_end_element(xmlns, "t"); write_end_element(xmlns, "si"); @@ -926,7 +920,7 @@ void xlsx_producer::write_shared_string_table(const relationship & /*rel*/) } write_start_element(xmlns, "t"); - write_characters(run.first, has_trailing_whitespace(run.first)); + write_characters(run.first, run.preserve_space); write_end_element(xmlns, "t"); write_end_element(xmlns, "r"); } diff --git a/source/styles/font.cpp b/source/styles/font.cpp index b474ab07..6bd31783 100644 --- a/source/styles/font.cpp +++ b/source/styles/font.cpp @@ -27,7 +27,11 @@ #include namespace { -const std::string Default_Name = "Calibri"; +const std::string &Default_Name() +{ + static const std::string Default("Calibri"); + return Default; +} constexpr double Default_Size = 12.0; } // namespace @@ -161,13 +165,13 @@ font &font::name(const std::string &name) return *this; } -const std::string& font::name() const +const std::string &font::name() const { if (name_.is_set()) { return name_.get(); } - return Default_Name; + return Default_Name(); } bool font::has_color() const @@ -229,7 +233,7 @@ std::size_t font::family() const return family_.get(); } -const std::string& font::scheme() const +const std::string &font::scheme() const { return scheme_.get(); } diff --git a/source/worksheet/phonetic_pr.cpp b/source/worksheet/phonetic_pr.cpp index 97efe752..602fa01c 100644 --- a/source/worksheet/phonetic_pr.cpp +++ b/source/worksheet/phonetic_pr.cpp @@ -25,18 +25,27 @@ #include namespace { // Order of elements defined by phonetic_pr::Type enum -const std::array Types{ - "fullwidthKatakana", - "halfwidthKatakana", - "Hiragana", - "noConversion"}; +const std::array& Types() +{ + static const std::array types{ + "fullwidthKatakana", + "halfwidthKatakana", + "Hiragana", + "noConversion" + }; + return types; +} // Order of elements defined by phonetic_pr::alignment enum -const std::array alignments{ - "Center", - "Distributed", - "Left", - "NoControl"}; +const std::array &Alignments() +{ + static const std::array alignments{ + "Center", + "Distributed", + "Left", + "NoControl"}; + return alignments; +} } // namespace @@ -108,14 +117,14 @@ void phonetic_pr::alignment(align align) // serialisation const std::string &phonetic_pr::type_as_string(phonetic_pr::phonetic_type type) { - return Types[static_cast(type)]; + return Types()[static_cast(type)]; } phonetic_pr::phonetic_type phonetic_pr::type_from_string(const std::string &str) { - for (std::size_t i = 0; i < Types.size(); ++i) + for (std::size_t i = 0; i < Types().size(); ++i) { - if (str == Types[i]) + if (str == Types()[i]) { return static_cast(i); } @@ -125,14 +134,14 @@ phonetic_pr::phonetic_type phonetic_pr::type_from_string(const std::string &str) const std::string &phonetic_pr::alignment_as_string(align type) { - return alignments[static_cast(type)]; + return Alignments()[static_cast(type)]; } phonetic_pr::align phonetic_pr::alignment_from_string(const std::string &str) { - for (std::size_t i = 0; i < alignments.size(); ++i) + for (std::size_t i = 0; i < Alignments().size(); ++i) { - if (str == alignments[i]) + if (str == Alignments()[i]) { return static_cast(i); } From ea850b32d52bbdc436f1bad71e7a2367c5524721 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 14 Jul 2018 19:59:03 +1200 Subject: [PATCH 20/38] Add clang 4.0 to travis builds --- .travis.yml | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index f89d204d..07c4db3f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -30,7 +30,7 @@ matrix: - STATIC=ON - SAMPLES=OFF - BENCHMARKS=OFF - # gcc-6, c++11, debug build, dll + # gcc-6, c++11, debug build, dynamic linking - os: linux compiler: gcc addons: @@ -64,6 +64,24 @@ matrix: - STATIC=ON - SAMPLES=ON - BENCHMARKS=ON + # clang 4, c++11, release build, dynamic linking, samples + benchmarks compiled + - os: linux + compiler: clang + addons: + apt: + sources: + - ubuntu-toolchain-r-test + - llvm-toolchain-trusty-4.0 + packages: + - clang-4.0 + env: + - CXX_COMPILER=clang++-4.0 + - C_COMPILER=clang-4.0 + - BUILD_TYPE=Release + - COVERAGE=OFF + - STATIC=OFF + - SAMPLES=ON + - BENCHMARKS=ON # clang 6, c++11, release build, static linking, samples + benchmarks compiled - os: linux compiler: clang From 11573f45e6811d6f930693310f9724c1e8150aab Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 14 Jul 2018 20:07:48 +1200 Subject: [PATCH 21/38] Resolve remaining warnings --- include/xlnt/worksheet/phonetic_pr.hpp | 2 +- source/CMakeLists.txt | 1 + source/detail/serialization/xlsx_producer.cpp | 4 +-- source/worksheet/phonetic_pr.cpp | 30 ++++++++++--------- source/worksheet/worksheet.cpp | 2 +- 5 files changed, 21 insertions(+), 18 deletions(-) diff --git a/include/xlnt/worksheet/phonetic_pr.hpp b/include/xlnt/worksheet/phonetic_pr.hpp index 592cb8e8..74561f9c 100644 --- a/include/xlnt/worksheet/phonetic_pr.hpp +++ b/include/xlnt/worksheet/phonetic_pr.hpp @@ -38,7 +38,7 @@ namespace xlnt { class XLNT_API phonetic_pr { public: - static const std::string Serialised_ID; + static std::string Serialised_ID(); /// /// possible values for alignment property diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 739d2c5a..24daabcc 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -46,6 +46,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-float-equal") # don't warn on uses of == for fp types set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-newline-eof") # no longer an issue with post-c++11 standards which mandate include add a newline if neccesary set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-covered-switch-default") # default is often added to switches for completeness or to cover future alternatives + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-exit-time-destructors") # this is just a warning to notify that the destructor will run during exit endif() if(STATIC_CRT) diff --git a/source/detail/serialization/xlsx_producer.cpp b/source/detail/serialization/xlsx_producer.cpp index d2c27a92..5d854d7d 100644 --- a/source/detail/serialization/xlsx_producer.cpp +++ b/source/detail/serialization/xlsx_producer.cpp @@ -2749,7 +2749,7 @@ void xlsx_producer::write_worksheet(const relationship &rel) if (ws.has_phonetic_properties()) { - write_start_element(xmlns, phonetic_pr::Serialised_ID); + write_start_element(xmlns, phonetic_pr::Serialised_ID()); const auto &ph_props = ws.phonetic_properties(); write_attribute("fontId", ph_props.font_id()); if (ph_props.has_type()) @@ -2760,7 +2760,7 @@ void xlsx_producer::write_worksheet(const relationship &rel) { write_attribute("alignment", phonetic_pr::alignment_as_string(ph_props.alignment())); } - write_end_element(xmlns, phonetic_pr::Serialised_ID); + write_end_element(xmlns, phonetic_pr::Serialised_ID()); } if (ws.has_page_margins()) diff --git a/source/worksheet/phonetic_pr.cpp b/source/worksheet/phonetic_pr.cpp index 602fa01c..f046fa58 100644 --- a/source/worksheet/phonetic_pr.cpp +++ b/source/worksheet/phonetic_pr.cpp @@ -25,14 +25,13 @@ #include namespace { // Order of elements defined by phonetic_pr::Type enum -const std::array& Types() +const std::array &Types() { static const std::array types{ - "fullwidthKatakana", - "halfwidthKatakana", - "Hiragana", - "noConversion" - }; + std::string("fullwidthKatakana"), + std::string("halfwidthKatakana"), + std::string("Hiragana"), + std::string("noConversion")}; return types; } @@ -40,10 +39,10 @@ const std::array& Types() const std::array &Alignments() { static const std::array alignments{ - "Center", - "Distributed", - "Left", - "NoControl"}; + std::string("Center"), + std::string("Distributed"), + std::string("Left"), + std::string("NoControl")}; return alignments; } @@ -53,7 +52,10 @@ namespace xlnt { /// /// out of line initialiser for static const member /// -const std::string phonetic_pr::Serialised_ID = "phoneticPr"; +std::string phonetic_pr::Serialised_ID() +{ + return "phoneticPr"; +} phonetic_pr::phonetic_pr(font_id_t font) : font_id_(font) @@ -62,7 +64,7 @@ phonetic_pr::phonetic_pr(font_id_t font) void phonetic_pr::serialise(std::ostream &output_stream) const { - output_stream << '<' << Serialised_ID << R"( fontID=")" << std::to_string(font_id_) << '"'; + output_stream << '<' << Serialised_ID() << R"( fontID=")" << std::to_string(font_id_) << '"'; if (has_type()) { output_stream << R"( type=")" << type_as_string(type_.get()) << '"'; @@ -117,7 +119,7 @@ void phonetic_pr::alignment(align align) // serialisation const std::string &phonetic_pr::type_as_string(phonetic_pr::phonetic_type type) { - return Types()[static_cast(type)]; + return Types()[static_cast(type)]; } phonetic_pr::phonetic_type phonetic_pr::type_from_string(const std::string &str) @@ -134,7 +136,7 @@ phonetic_pr::phonetic_type phonetic_pr::type_from_string(const std::string &str) const std::string &phonetic_pr::alignment_as_string(align type) { - return Alignments()[static_cast(type)]; + return Alignments()[static_cast(type)]; } phonetic_pr::align phonetic_pr::alignment_from_string(const std::string &str) diff --git a/source/worksheet/worksheet.cpp b/source/worksheet/worksheet.cpp index 9d59e895..5ab28fa1 100644 --- a/source/worksheet/worksheet.cpp +++ b/source/worksheet/worksheet.cpp @@ -98,7 +98,7 @@ void worksheet::create_named_range(const std::string &name, const range_referenc throw invalid_parameter(); //("named range name must be outside the range A1-XFD1048576"); } } - catch (xlnt::invalid_cell_reference) + catch (xlnt::invalid_cell_reference&) { // name is not a valid reference, that's good } From 6b0fb72e78244e31114544140495ac1397b9651f Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 14 Jul 2018 20:26:26 +1200 Subject: [PATCH 22/38] make benchmarking conditional on other flags (static && release) --- .travis.yml | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/.travis.yml b/.travis.yml index 07c4db3f..43e90c03 100644 --- a/.travis.yml +++ b/.travis.yml @@ -29,7 +29,7 @@ matrix: - COVERAGE=ON - STATIC=ON - SAMPLES=OFF - - BENCHMARKS=OFF + # gcc-6, c++11, debug build, dynamic linking - os: linux compiler: gcc @@ -42,11 +42,11 @@ matrix: env: - CXX_COMPILER=g++-6 - C_COMPILER=gcc-6 - - BUILD_TYPE=Debug + - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=OFF - SAMPLES=OFF - - BENCHMARKS=OFF + # gcc-8, c++11, release build, static linking, samples + benchmarks compiled - os: linux compiler: gcc @@ -63,7 +63,7 @@ matrix: - COVERAGE=OFF - STATIC=ON - SAMPLES=ON - - BENCHMARKS=ON + # clang 4, c++11, release build, dynamic linking, samples + benchmarks compiled - os: linux compiler: clang @@ -80,8 +80,8 @@ matrix: - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=OFF - - SAMPLES=ON - - BENCHMARKS=ON + - SAMPLES=OFF + # clang 6, c++11, release build, static linking, samples + benchmarks compiled - os: linux compiler: clang @@ -99,27 +99,32 @@ matrix: - COVERAGE=OFF - STATIC=ON - SAMPLES=ON - - BENCHMARKS=ON before_install: - export CC=${C_COMPILER} - export CXX=${CXX_COMPILER} install: - - | - if [[ "${COVERAGE}" == "ON" ]]; then - gem install coveralls-lcov; - fi - - - ${CXX} --version - - cmake --version + - | + if [[ "${COVERAGE}" == "ON" ]]; then + gem install coveralls-lcov; + fi + + - ${CXX} --version + - cmake --version script: - - mkdir build - - cd build - - cmake -D STATIC=$STATIC -D BENCHMARKS=$BENCHMARKS -D SAMPLES=$SAMPLES -D COVERAGE=$COVERAGE -D CMAKE_BUILD_TYPE=$BUILD_TYPE .. - - cmake --build . -- -j2 - - ./tests/xlnt.test + - | + if [[ "${BUILD_TYPE}" == "Release" && "${STATIC}" == "ON" ]]; + then BENCHMARKS="ON"; + else BENCHMARKS="OFF"; + fi + + - mkdir build + - cd build + - cmake -D STATIC=$STATIC -D BENCHMARKS=$BENCHMARKS -D SAMPLES=$SAMPLES -D COVERAGE=$COVERAGE -D CMAKE_BUILD_TYPE=$BUILD_TYPE .. + - cmake --build . -- -j2 + - ./tests/xlnt.test after_success: - | From 25d75cb5c327c13a549c331cd1011fb2b9750b17 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 14 Jul 2018 21:19:54 +1200 Subject: [PATCH 23/38] complete the compiler set (gcc 6/7/8 + clang 4/5/6) --- .travis.yml | 46 ++++++++++++++++++++++++++++++++++++++----- source/CMakeLists.txt | 1 + 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index 43e90c03..31b2a0b9 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,6 +12,7 @@ notifications: # set up build matrix matrix: include: + # ============= CODE COVERAGE =============== # gcc-6, c++11, debug build, static linking, code coverage enabled - os: linux compiler: gcc @@ -29,7 +30,8 @@ matrix: - COVERAGE=ON - STATIC=ON - SAMPLES=OFF - + + # ============= GCC ================== # gcc-6, c++11, debug build, dynamic linking - os: linux compiler: gcc @@ -46,7 +48,24 @@ matrix: - COVERAGE=OFF - STATIC=OFF - SAMPLES=OFF - + + # gcc-8, c++11, release build, static linking, samples + benchmarks compiled + - os: linux + compiler: gcc + addons: + apt: + sources: + - ubuntu-toolchain-r-test + packages: + - g++-7 + env: + - CXX_COMPILER=g++-7 + - C_COMPILER=gcc-7 + - BUILD_TYPE=Release + - COVERAGE=OFF + - STATIC=ON + - SAMPLES=ON + # gcc-8, c++11, release build, static linking, samples + benchmarks compiled - os: linux compiler: gcc @@ -64,7 +83,8 @@ matrix: - STATIC=ON - SAMPLES=ON - # clang 4, c++11, release build, dynamic linking, samples + benchmarks compiled + # =========== CLANG ============= + # clang 4, c++11, release build, dynamic linking - os: linux compiler: clang addons: @@ -81,8 +101,24 @@ matrix: - COVERAGE=OFF - STATIC=OFF - SAMPLES=OFF - - # clang 6, c++11, release build, static linking, samples + benchmarks compiled + # clang 5, c++11, release build, dynamic linking, samples + benchmarks compiled + - os: linux + compiler: clang + addons: + apt: + sources: + - ubuntu-toolchain-r-test + - llvm-toolchain-trusty-5.0 + packages: + - clang-5.0 + env: + - CXX_COMPILER=clang++-5.0 + - C_COMPILER=clang-5.0 + - BUILD_TYPE=Release + - COVERAGE=OFF + - STATIC=ON + - SAMPLES=ON + # clang 6, c++11, release build, static linking, samples compiled - os: linux compiler: clang addons: diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 24daabcc..c826e3b0 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -47,6 +47,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-newline-eof") # no longer an issue with post-c++11 standards which mandate include add a newline if neccesary set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-covered-switch-default") # default is often added to switches for completeness or to cover future alternatives set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-exit-time-destructors") # this is just a warning to notify that the destructor will run during exit + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-missing-braces") # Wmissing-field-initializers has less false positives endif() if(STATIC_CRT) From 5671167d1da2373fa43ca56a2ea0eed93078d990 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sun, 15 Jul 2018 21:23:22 +1200 Subject: [PATCH 24/38] add cmake option XLNT_CXX_LANG to set the targetted cxx standard - valid options are 11, 14, and 17 - default is 14 - cmake will error if an invalid value is provided - requires cmake >= 3.10.* to take effect in visual studio --- .travis.yml | 57 +++++++++++++++++++++++++------------------ CMakeLists.txt | 12 +++++++++ source/CMakeLists.txt | 42 +++++++++++++++---------------- tests/CMakeLists.txt | 5 ++-- 4 files changed, 67 insertions(+), 49 deletions(-) diff --git a/.travis.yml b/.travis.yml index 31b2a0b9..0e25f336 100644 --- a/.travis.yml +++ b/.travis.yml @@ -12,25 +12,6 @@ notifications: # set up build matrix matrix: include: - # ============= CODE COVERAGE =============== - # gcc-6, c++11, debug build, static linking, code coverage enabled - - os: linux - compiler: gcc - addons: - apt: - sources: - - ubuntu-toolchain-r-test - packages: - - g++-6 - - lcov - env: - - CXX_COMPILER=g++-6 - - C_COMPILER=gcc-6 - - BUILD_TYPE=Debug - - COVERAGE=ON - - STATIC=ON - - SAMPLES=OFF - # ============= GCC ================== # gcc-6, c++11, debug build, dynamic linking - os: linux @@ -44,12 +25,13 @@ matrix: env: - CXX_COMPILER=g++-6 - C_COMPILER=gcc-6 + - CXX_VER=11 - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=OFF - SAMPLES=OFF - # gcc-8, c++11, release build, static linking, samples + benchmarks compiled + # gcc-7, c++14, release build, static linking, samples + benchmarks compiled - os: linux compiler: gcc addons: @@ -61,12 +43,13 @@ matrix: env: - CXX_COMPILER=g++-7 - C_COMPILER=gcc-7 + - CXX_VER=14 - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=ON - SAMPLES=ON - # gcc-8, c++11, release build, static linking, samples + benchmarks compiled + # gcc-8, c++17, release build, static linking, samples + benchmarks compiled - os: linux compiler: gcc addons: @@ -78,6 +61,7 @@ matrix: env: - CXX_COMPILER=g++-8 - C_COMPILER=gcc-8 + - CXX_VER=17 - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=ON @@ -97,11 +81,13 @@ matrix: env: - CXX_COMPILER=clang++-4.0 - C_COMPILER=clang-4.0 + - CXX_VER=11 - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=OFF - SAMPLES=OFF - # clang 5, c++11, release build, dynamic linking, samples + benchmarks compiled + + # clang 5, c++14, release build, dynamic linking, samples + benchmarks compiled - os: linux compiler: clang addons: @@ -114,11 +100,13 @@ matrix: env: - CXX_COMPILER=clang++-5.0 - C_COMPILER=clang-5.0 + - CXX_VER=14 - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=ON - SAMPLES=ON - # clang 6, c++11, release build, static linking, samples compiled + + # clang 6, c++17, release build, static linking, samples compiled - os: linux compiler: clang addons: @@ -131,10 +119,31 @@ matrix: env: - CXX_COMPILER=clang++-6.0 - C_COMPILER=clang-6.0 + - CXX_VER=17 - BUILD_TYPE=Release - COVERAGE=OFF - STATIC=ON - SAMPLES=ON + + # ============= CODE COVERAGE =============== + # gcc-6, c++11, debug build, static linking, code coverage enabled + - os: linux + compiler: gcc + addons: + apt: + sources: + - ubuntu-toolchain-r-test + packages: + - g++-6 + - lcov + env: + - CXX_COMPILER=g++-6 + - C_COMPILER=gcc-6 + - CXX_VER=11 + - BUILD_TYPE=Debug + - COVERAGE=ON + - STATIC=ON + - SAMPLES=OFF before_install: - export CC=${C_COMPILER} @@ -158,7 +167,7 @@ script: - mkdir build - cd build - - cmake -D STATIC=$STATIC -D BENCHMARKS=$BENCHMARKS -D SAMPLES=$SAMPLES -D COVERAGE=$COVERAGE -D CMAKE_BUILD_TYPE=$BUILD_TYPE .. + - cmake .. -DXLNT_CXX_LANG=${CXX_VER} -DSTATIC=$STATIC -DBENCHMARKS=$BENCHMARKS -DSAMPLES=$SAMPLES -DCOVERAGE=$COVERAGE -DCMAKE_BUILD_TYPE=$BUILD_TYPE - cmake --build . -- -j2 - ./tests/xlnt.test diff --git a/CMakeLists.txt b/CMakeLists.txt index 0761378d..8c4a8d65 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,6 +7,18 @@ set(COMBINED_PROJECT TRUE) # Library type option(STATIC "Set to ON to build xlnt as a static library instead of a shared library" OFF) +# c++ language standard to use +set(XLNT_LANGS 11 14 17) +set(XLNT_CXX_LANG "14" CACHE STRING "c++ language features to compile with") +# enumerate allowed values for cmake gui +set_property(CACHE XLNT_CXX_LANG PROPERTY STRINGS ${XLNT_LANGS}) +# validate value is in XLNT_LANGS +list(FIND XLNT_LANGS ${XLNT_CXX_LANG} index) +if(index EQUAL -1) + message(FATAL_ERROR "XLNT_CXX_LANG must be one of ${XLNT_LANGS}") +endif() + + # Optional components option(TESTS "Set to OFF to skip building test executable (in ./tests)" ON) option(SAMPLES "Set to ON to build executable code samples (in ./samples)" OFF) diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index c826e3b0..6de34969 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -1,11 +1,7 @@ cmake_minimum_required(VERSION 3.1) project(xlnt VERSION 1.2) -# Require C99 and C++11 compilers -set(CMAKE_C_STANDARD 99) -set(CMAKE_C_STANDARD_REQUIRED ON) -set(CMAKE_CXX_STANDARD 11) -set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CXX_EXTENSIONS OFF) # Project metadata set(PROJECT_VENDOR "Thomas Fussell") @@ -156,6 +152,8 @@ else() target_compile_definitions(xlnt PUBLIC XLNT_STATIC=1) endif() +target_compile_features(xlnt PUBLIC cxx_std_${XLNT_CXX_LANG}) + # Includes target_include_directories(xlnt PUBLIC ${XLNT_INCLUDE_DIR}) target_include_directories(xlnt PRIVATE ${XLNT_SOURCE_DIR}) @@ -178,23 +176,23 @@ if(MSVC) set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/detail/cryptography/aes.cpp PROPERTIES COMPILE_FLAGS "/wd\"4996\"") -endif() - -# Platform- and file-specific settings, Clang -if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") - set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/detail/serialization/miniz.cpp - PROPERTIES - COMPILE_FLAGS "-Wno-undef") - set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/detail/serialization/zstream.cpp - PROPERTIES - COMPILE_FLAGS "-Wno-undef -Wno-shorten-64-to-32") -endif() - -# Platform- and file-specific settings, GCC -if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") - set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/detail/serialization/miniz.cpp - PROPERTIES - COMPILE_FLAGS "-Wno-strict-aliasing") +else() + # Platform- and file-specific settings, Clang + if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") + set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/detail/serialization/miniz.cpp + PROPERTIES + COMPILE_FLAGS "-Wno-undef") + set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/detail/serialization/zstream.cpp + PROPERTIES + COMPILE_FLAGS "-Wno-undef -Wno-shorten-64-to-32") + endif() + + # Platform- and file-specific settings, GCC + if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") + set_source_files_properties(${CMAKE_CURRENT_SOURCE_DIR}/detail/serialization/miniz.cpp + PROPERTIES + COMPILE_FLAGS "-Wno-strict-aliasing") + endif() endif() # Group files into pseudo-folders in IDEs diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b472636d..40fcc124 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,9 +1,7 @@ cmake_minimum_required(VERSION 3.1) project(xlnt.test) -# Require C++11 compiler -set(CMAKE_CXX_STANDARD 11) -set(CMAKE_CXX_STANDARD_REQUIRED ON) +set(CXX_EXTENSIONS OFF) if(NOT COMBINED_PROJECT) add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/../source ${CMAKE_CURRENT_BINARY_DIR}/source) @@ -48,6 +46,7 @@ target_include_directories(xlnt.test set(XLNT_TEST_DATA_DIR ${CMAKE_CURRENT_SOURCE_DIR}/data) target_compile_definitions(xlnt.test PRIVATE XLNT_TEST_DATA_DIR=${XLNT_TEST_DATA_DIR}) +target_compile_features(xlnt.test PRIVATE cxx_std_${XLNT_CXX_LANG}) if(MSVC) # bigobj because there are so many headers in one source file From b9b47672ea07c2afdcf3b2b528dc489e5c5eb5ed Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 16 Jul 2018 18:52:21 +1200 Subject: [PATCH 25/38] cxx_std_14 and co aren't available until cmake v3.8 --- source/CMakeLists.txt | 4 +++- tests/CMakeLists.txt | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 6de34969..90568121 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -1,6 +1,8 @@ cmake_minimum_required(VERSION 3.1) project(xlnt VERSION 1.2) +set(CMAKE_CXX_STANDARD ${XLNT_CXX_LANG}) +set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CXX_EXTENSIONS OFF) # Project metadata @@ -152,7 +154,7 @@ else() target_compile_definitions(xlnt PUBLIC XLNT_STATIC=1) endif() -target_compile_features(xlnt PUBLIC cxx_std_${XLNT_CXX_LANG}) +#target_compile_features(xlnt PUBLIC cxx_std_${XLNT_CXX_LANG}) # Includes target_include_directories(xlnt PUBLIC ${XLNT_INCLUDE_DIR}) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 40fcc124..d6b9361b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,6 +1,8 @@ cmake_minimum_required(VERSION 3.1) project(xlnt.test) +set(CMAKE_CXX_STANDARD ${XLNT_CXX_LANG}) +set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CXX_EXTENSIONS OFF) if(NOT COMBINED_PROJECT) @@ -46,7 +48,7 @@ target_include_directories(xlnt.test set(XLNT_TEST_DATA_DIR ${CMAKE_CURRENT_SOURCE_DIR}/data) target_compile_definitions(xlnt.test PRIVATE XLNT_TEST_DATA_DIR=${XLNT_TEST_DATA_DIR}) -target_compile_features(xlnt.test PRIVATE cxx_std_${XLNT_CXX_LANG}) +#target_compile_features(xlnt.test PRIVATE cxx_std_${XLNT_CXX_LANG}) if(MSVC) # bigobj because there are so many headers in one source file From ad759ae4f7979d5a031672e03d02a82d03b6449a Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 16 Jul 2018 18:59:00 +1200 Subject: [PATCH 26/38] fix for build failure with GCC8{-std=c++17} --- source/detail/serialization/zstream.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/source/detail/serialization/zstream.cpp b/source/detail/serialization/zstream.cpp index 9888d316..be72bd50 100644 --- a/source/detail/serialization/zstream.cpp +++ b/source/detail/serialization/zstream.cpp @@ -366,7 +366,7 @@ public: deflateEnd(&strm); if (header) { - std::ios::streampos final_position = ostream.tellp(); + auto final_position = ostream.tellp(); header->uncompressed_size = uncompressed_size; header->crc = crc; ostream.seekp(header->header_offset); @@ -457,14 +457,14 @@ ozstream::ozstream(std::ostream &stream) ozstream::~ozstream() { // Write all file headers - std::ios::streampos final_position = destination_stream_.tellp(); + auto final_position = destination_stream_.tellp(); for (const auto &header : file_headers_) { write_header(header, destination_stream_, true); } - std::ios::streampos central_end = destination_stream_.tellp(); + auto central_end = destination_stream_.tellp(); // Write end of central write_int(destination_stream_, static_cast(0x06054b50)); // end of central @@ -507,12 +507,12 @@ bool izstream::read_central_header() // Find the header // NOTE: this assumes the zip file header is the last thing written to file... source_stream_.seekg(0, std::ios_base::end); - std::ios::streampos end_position = source_stream_.tellg(); + auto end_position = static_cast(source_stream_.tellg()); auto max_comment_size = std::uint32_t(0xffff); // max size of header auto read_size_before_comment = std::uint32_t(22); - std::ios::streamoff read_start = max_comment_size + read_size_before_comment; + std::uint32_t read_start = max_comment_size + read_size_before_comment; if (read_start > end_position) { From 0aee6fd9b152a18563583f116049756582cd2816 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Mon, 16 Jul 2018 19:41:40 +1200 Subject: [PATCH 27/38] Cleanup some cmake modifications --- CMakeLists.txt | 10 +++++----- source/CMakeLists.txt | 1 + tests/CMakeLists.txt | 1 + 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8c4a8d65..d2a50996 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8,14 +8,14 @@ set(COMBINED_PROJECT TRUE) option(STATIC "Set to ON to build xlnt as a static library instead of a shared library" OFF) # c++ language standard to use -set(XLNT_LANGS 11 14 17) +set(XLNT_VALID_LANGS 11 14 17) set(XLNT_CXX_LANG "14" CACHE STRING "c++ language features to compile with") # enumerate allowed values for cmake gui -set_property(CACHE XLNT_CXX_LANG PROPERTY STRINGS ${XLNT_LANGS}) -# validate value is in XLNT_LANGS -list(FIND XLNT_LANGS ${XLNT_CXX_LANG} index) +set_property(CACHE XLNT_CXX_LANG PROPERTY STRINGS ${XLNT_VALID_LANGS}) +# validate value is in XLNT_VALID_LANGS +list(FIND XLNT_VALID_LANGS ${XLNT_CXX_LANG} index) if(index EQUAL -1) - message(FATAL_ERROR "XLNT_CXX_LANG must be one of ${XLNT_LANGS}") + message(FATAL_ERROR "XLNT_CXX_LANG must be one of ${XLNT_VALID_LANGS}") endif() diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 90568121..2ea048d3 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -154,6 +154,7 @@ else() target_compile_definitions(xlnt PUBLIC XLNT_STATIC=1) endif() +# requires cmake 3.8+ #target_compile_features(xlnt PUBLIC cxx_std_${XLNT_CXX_LANG}) # Includes diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d6b9361b..74c24352 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -48,6 +48,7 @@ target_include_directories(xlnt.test set(XLNT_TEST_DATA_DIR ${CMAKE_CURRENT_SOURCE_DIR}/data) target_compile_definitions(xlnt.test PRIVATE XLNT_TEST_DATA_DIR=${XLNT_TEST_DATA_DIR}) +# requires cmake 3.8+ #target_compile_features(xlnt.test PRIVATE cxx_std_${XLNT_CXX_LANG}) if(MSVC) From 4f9e8ab37f152ffe0afec80083cc831ea3b3f387 Mon Sep 17 00:00:00 2001 From: sukoi26 Date: Thu, 19 Jul 2018 12:49:32 +0200 Subject: [PATCH 28/38] bestFit change remove from skip_attributes list --- source/detail/serialization/xlsx_consumer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/detail/serialization/xlsx_consumer.cpp b/source/detail/serialization/xlsx_consumer.cpp index e15ef435..edfc63b2 100644 --- a/source/detail/serialization/xlsx_consumer.cpp +++ b/source/detail/serialization/xlsx_consumer.cpp @@ -601,7 +601,7 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) { expect_start_element(qn("spreadsheetml", "col"), xml::content::simple); - skip_attributes({"bestFit", "collapsed", "outlineLevel"}); + skip_attributes({"collapsed", "outlineLevel"}); auto min = static_cast(std::stoull(parser().attribute("min"))); auto max = static_cast(std::stoull(parser().attribute("max"))); From a2dc4a34f134870ed00d59b66c4dbeadeb5b1d8a Mon Sep 17 00:00:00 2001 From: sukoi26 Date: Thu, 19 Jul 2018 13:53:54 +0200 Subject: [PATCH 29/38] update bestFit commit error compiling std::vector --- source/detail/serialization/xlsx_consumer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/detail/serialization/xlsx_consumer.cpp b/source/detail/serialization/xlsx_consumer.cpp index edfc63b2..336ddaa3 100644 --- a/source/detail/serialization/xlsx_consumer.cpp +++ b/source/detail/serialization/xlsx_consumer.cpp @@ -601,7 +601,7 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) { expect_start_element(qn("spreadsheetml", "col"), xml::content::simple); - skip_attributes({"collapsed", "outlineLevel"}); + skip_attributes(std::vector{"collapsed", "outlineLevel"}); auto min = static_cast(std::stoull(parser().attribute("min"))); auto max = static_cast(std::stoull(parser().attribute("max"))); From eda102ee9dc1c544fb5404e625c561f71443d145 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:22:30 +1200 Subject: [PATCH 30/38] testing current optional implementation against target behaviour Issue #300 -- NOTE: construction from no_default currently wont compile so is commented out (L:113) -- NOTE: dll import/export of template classes is probably unnecessary (the header is already required) and doesn't work in the general case (explicit instantiations are required for at least MSVC) --- include/xlnt/utils/optional.hpp | 12 +- tests/utils/optional_tests.cpp | 304 ++++++++++++++++++++++++++++++++ 2 files changed, 315 insertions(+), 1 deletion(-) create mode 100644 tests/utils/optional_tests.cpp diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 143c38ec..1978009b 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -34,7 +34,7 @@ namespace xlnt { /// within the optional class. /// template -class XLNT_API optional +class optional { public: /// @@ -135,6 +135,16 @@ public: return value_ == other.value_; } + /// + /// Returns false if neither this nor other have a value + /// or both have a value and those values are equal according to + /// their equality operator. + /// + bool operator!=(const optional &other) const + { + return !operator==(other); + } + private: bool has_value_; T value_; diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp new file mode 100644 index 00000000..b17cbecb --- /dev/null +++ b/tests/utils/optional_tests.cpp @@ -0,0 +1,304 @@ +// Copyright (c) 2014-2018 Thomas Fussell +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, WRISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE +// +// @license: http://www.opensource.org/licenses/mit-license.php +// @author: see AUTHORS file + +#pragma once + +#include +#include + +// test helpers +namespace { +// increments count when constructed, decrements when destructed +// use to ensure correct ctor/dtor pairing +struct alive_count +{ + alive_count() + { + ++count; + } + + alive_count(const alive_count &) + { + ++count; + } + + alive_count(alive_count &&) + { + ++count; + } + + ~alive_count() + { + --count; + } + + alive_count &operator=(const alive_count &) = default; + alive_count &operator=(alive_count &&) = default; + + static int count; +}; +int alive_count::count = 0; + +// implicitly convertible from int +struct convertible +{ + // implicit construction from int + convertible(int i) + : val(i) + { + } + + int val; +}; + +// default ctor deleted +struct no_default +{ + no_default() = delete; + int i; +}; +} // namespace + +class optional_test_suite : public test_suite +{ +public: + optional_test_suite() + : test_suite() + { + register_test(test_ctor); + register_test(test_copy_ctor); + register_test(test_move_ctor); + register_test(test_copy_assign); + register_test(test_move_assign); + register_test(test_set_and_get); + register_test(test_equality); + register_test(test_const); + } + + void test_ctor() + { + // default + xlnt::optional opt1; + xlnt_assert(!opt1.is_set()); + // value + const int test_val = 3; + xlnt::optional opt2(test_val); + xlnt_assert(opt2.is_set()); + xlnt_assert_equals(opt2.get(), test_val); + // converting + xlnt::optional opt3(test_val); + xlnt_assert(opt3.is_set()); + xlnt_assert_equals(opt3.get().val, test_val); + //// no default ctor + //xlnt::optional no_def_opt; + } + + void test_copy_ctor() + { + { // copy behaviour + xlnt::optional opt1; + xlnt::optional opt2(opt1); + xlnt_assert_equals(opt1, opt2); + + const int test_val = 123; + xlnt::optional opt3(test_val); + xlnt::optional opt4(opt3); + xlnt_assert_equals(opt3, opt4); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt2(opt1); + xlnt_assert_equals(2, alive_count::count); + } + xlnt_assert_equals(1, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_move_ctor() + { + { // move behaviour + xlnt::optional opt1; + xlnt::optional opt2(std::move(opt1)); + xlnt_assert_equals(opt2, xlnt::optional{}); // can't test against opt1 so use a temporary + + const int test_val = 123; + xlnt::optional opt3(test_val); + xlnt::optional opt4(std::move(opt3)); + xlnt_assert(opt4.is_set()); // moved to optional contains the value + xlnt_assert_equals(opt4.get(), test_val); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt2(std::move(opt1)); + xlnt_assert_equals(1, alive_count::count); // opt1 is in a no-value state + } + xlnt_assert_equals(0, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_copy_assign() + { + { // copy assign behaviour + xlnt::optional opt1; + xlnt::optional opt_assign1; // to actually test assignment, the value needs to be already created. using '=' is not enough + opt_assign1 = opt1; + xlnt_assert_equals(opt1, opt_assign1); + + const int test_val = 123; + xlnt::optional opt2(test_val); + xlnt::optional opt_assign2; + opt_assign2 = opt2; + xlnt_assert_equals(opt2, opt_assign2); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt_assign1; + opt_assign1 = opt1; + xlnt_assert_equals(2, alive_count::count); + } + xlnt_assert_equals(1, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_move_assign() + { + { // copy assign behaviour + xlnt::optional opt1; + xlnt::optional opt_assign1; // to actually test assignment, the value needs to be already created. using '=' is not enough + opt_assign1 = std::move(opt1); + xlnt_assert_equals(opt_assign1, xlnt::optional{}); // can't test against opt1 so use a temporary + + const int test_val = 123; + xlnt::optional opt2(test_val); + xlnt::optional opt_assign2; + opt_assign2 = std::move(opt2); + xlnt_assert(opt_assign2.is_set()); // moved to optional contains the value + xlnt_assert_equals(opt_assign2.get(), test_val); + } + { // lifetime checks + xlnt::optional opt1(alive_count{}); + xlnt_assert_equals(1, alive_count::count); + { + xlnt::optional opt_assign1; + opt_assign1 = std::move(opt1); + xlnt_assert_equals(1, alive_count::count); // opt1 is in a no-value state + } + xlnt_assert_equals(0, alive_count::count); + } + xlnt_assert_equals(0, alive_count::count); // dtor test + } + + void test_set_and_get() + { + { + xlnt::optional test_opt; + xlnt_assert(!test_opt.is_set()); + xlnt_assert_throws(test_opt.get(), xlnt::invalid_attribute); + // set + const int test_val1 = 321; + test_opt.set(test_val1); + xlnt_assert(test_opt.is_set()); + xlnt_assert_equals(test_opt.get(), test_val1); + // set again + const int test_val2 = 123; + test_opt.set(test_val2); + xlnt_assert(test_opt.is_set()); + xlnt_assert_equals(test_opt.get(), test_val2); + // clear + test_opt.clear(); + xlnt_assert(!test_opt.is_set()); + xlnt_assert_throws(test_opt.get(), xlnt::invalid_attribute); + // set again + const int test_val3 = 3; + test_opt.set(test_val3); + xlnt_assert(test_opt.is_set()); + xlnt_assert_equals(test_opt.get(), test_val3); + } + { // lifetime checks + xlnt::optional test_opt; + xlnt_assert_equals(0, alive_count::count); + test_opt.set(alive_count()); + xlnt_assert_equals(1, alive_count::count); + test_opt.set(alive_count()); // reassignment doesn't somehow skip the dtor + xlnt_assert_equals(1, alive_count::count); + test_opt.clear(); + xlnt_assert_equals(0, alive_count::count); + } + } + + void test_equality() + { + xlnt::optional test_opt1; + xlnt::optional test_opt2; + // no value opts compare equal + xlnt_assert(test_opt1 == test_opt2); + xlnt_assert(!(test_opt1 != test_opt2)); + xlnt_assert(test_opt2 == test_opt1); + xlnt_assert(!(test_opt2 != test_opt1)); + // value compares false with no value + const int test_val = 1; + test_opt1.set(test_val); + xlnt_assert(test_opt1 != test_opt2); + xlnt_assert(!(test_opt1 == test_opt2)); + xlnt_assert(test_opt2 != test_opt1); + xlnt_assert(!(test_opt2 == test_opt1)); + // value compares false with a different value + const int test_val2 = 2; + test_opt2.set(test_val2); + xlnt_assert(test_opt1 != test_opt2); + xlnt_assert(!(test_opt1 == test_opt2)); + xlnt_assert(test_opt2 != test_opt1); + xlnt_assert(!(test_opt2 == test_opt1)); + // value compares equal with same value + test_opt2.set(test_val); + xlnt_assert(test_opt1 == test_opt2); + xlnt_assert(!(test_opt1 != test_opt2)); + xlnt_assert(test_opt2 == test_opt1); + xlnt_assert(!(test_opt2 != test_opt1)); + } + + void test_const() + { + // functions on a const optional + const int test_val = 1; + const xlnt::optional opt(test_val); + xlnt_assert(opt.is_set()); + xlnt_assert(opt.get() == test_val); + + xlnt::optional opt2(test_val); + xlnt_assert(opt == opt2); + xlnt_assert(opt2 == opt); + xlnt_assert(!(opt != opt2)); + xlnt_assert(!(opt2 != opt)); + } +}; +static optional_test_suite x; \ No newline at end of file From ad69e7bf11aeaf4b5aa990dccca66c4467d7591a Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:22:43 +1200 Subject: [PATCH 31/38] test for overloaded operator=(T) Issue #300 --- tests/utils/optional_tests.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp index b17cbecb..7af72a20 100644 --- a/tests/utils/optional_tests.cpp +++ b/tests/utils/optional_tests.cpp @@ -242,6 +242,10 @@ public: test_opt.set(test_val3); xlnt_assert(test_opt.is_set()); xlnt_assert_equals(test_opt.get(), test_val3); + // operator= set + xlnt::optional test_opt2; + test_opt2 = test_val1; + xlnt_assert_equals(test_opt2.get(), test_val1); } { // lifetime checks xlnt::optional test_opt; From 34e8f274def67e166ad57196be24ee8d45345d50 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:22:54 +1200 Subject: [PATCH 32/38] optional exception check, primarily adding noexcept to signatures -- also reordered the assignments in set/clear to ensure "has_value_" doesn't change if the assignment operator of T throws --- include/xlnt/utils/optional.hpp | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 1978009b..03415304 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -40,14 +40,17 @@ public: /// /// Default contructor. is_set() will be false initially. /// - optional() : has_value_(false), value_(T()) + optional() noexcept(std::is_nothrow_default_constructible{}) + : has_value_(false), value_(T()) { } /// /// Constructs this optional with a value. + /// noexcept if T copy ctor is noexcept /// - optional(const T &value) : has_value_(true), value_(value) + optional(const T &value) noexcept(std::is_nothrow_copy_constructible{}) + : has_value_(true), value_(value) { } @@ -55,7 +58,7 @@ public: /// Returns true if this object currently has a value set. This should /// be called before accessing the value with optional::get(). /// - bool is_set() const + bool is_set() const noexcept { return has_value_; } @@ -63,10 +66,10 @@ public: /// /// Sets the value to value. /// - void set(const T &value) + void set(const T &value) noexcept(std::is_nothrow_assignable{}) { - has_value_ = true; value_ = value; + has_value_ = true; } /// @@ -101,20 +104,18 @@ public: /// Resets the internal value using its default constructor. After this is /// called, is_set() will return false until a new value is provided. /// - void clear() + void clear() noexcept(std::is_nothrow_assignable{} && std::is_nothrow_default_constructible{}) { - has_value_ = false; value_ = T(); + has_value_ = false; } /// /// Assignment operator. Equivalent to setting the value using optional::set. /// - optional &operator=(const T &rhs) + optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) { - has_value_ = true; - value_ = rhs; - + set(rhs); return *this; } @@ -123,9 +124,10 @@ public: /// or both have a value and those values are equal according to /// their equality operator. /// - bool operator==(const optional &other) const + bool operator==(const optional &other) const noexcept { - if (has_value_ != other.has_value_) { + if (has_value_ != other.has_value_) + { return false; } if (!has_value_) @@ -140,7 +142,7 @@ public: /// or both have a value and those values are equal according to /// their equality operator. /// - bool operator!=(const optional &other) const + bool operator!=(const optional &other) const noexcept { return !operator==(other); } From 07d648fe8b6e27d2c78f1e4076a5b726b5d0ae84 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 7 Jul 2018 21:05:50 +1200 Subject: [PATCH 33/38] Implementation of optional using std::aligned_storage Issue #300 -- test for no default constructor required enabled -- Passes all tests locally --- include/xlnt/utils/optional.hpp | 190 +++++++++++++++++++++++++++----- tests/utils/optional_tests.cpp | 4 +- 2 files changed, 162 insertions(+), 32 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 03415304..75fd4ac5 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -40,8 +40,8 @@ public: /// /// Default contructor. is_set() will be false initially. /// - optional() noexcept(std::is_nothrow_default_constructible{}) - : has_value_(false), value_(T()) + optional() noexcept + : has_value_(false) { } @@ -50,8 +50,89 @@ public: /// noexcept if T copy ctor is noexcept /// optional(const T &value) noexcept(std::is_nothrow_copy_constructible{}) - : has_value_(true), value_(value) + : has_value_(true) { + new (&storage_) T(value); + } + + /// + /// Constructs this optional with a value. + /// noexcept if T move ctor is noexcept + /// + optional(T &&value) noexcept(std::is_nothrow_move_constructible{}) + : has_value_(true) + { + new (&storage_) T(std::move(value)); + } + + /// + /// Copy constructs this optional from other + /// noexcept if T copy ctor is noexcept + /// + optional(const optional& other) noexcept(std::is_nothrow_copy_constructible{}) + : has_value_(other.has_value_) + { + if (has_value_) + { + new (&storage_) T(other.value_ref()); + } + } + + /// + /// Move constructs this optional from other. Clears the value from other if set + /// noexcept if T move ctor is noexcept + /// + optional(optional &&other) noexcept(std::is_nothrow_move_constructible{}) + : has_value_(other.has_value_) + { + if (has_value_) + { + new (&storage_) T(std::move(other.value_ref())); + other.clear(); + } + } + + /// + /// Copy assignment of this optional from other + /// noexcept if set and clear are noexcept for T& + /// + optional &operator=(const optional &other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + { + if (other.has_value_) + { + set(other.value_ref()); + } + else + { + clear(); + } + return *this; + } + + /// + /// Move assignment of this optional from other + /// noexcept if set and clear are noexcept for T&& + /// + optional &operator=(optional &&other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + { + if (other.has_value_) + { + set(std::move(other.value_ref())); + other.clear(); + } + else + { + clear(); + } + return *this; + } + + /// + /// Destructor cleans up the T instance if set + /// + ~optional() noexcept // note:: unconditional because msvc freaks out otherwise + { + clear(); } /// @@ -64,12 +145,69 @@ public: } /// - /// Sets the value to value. + /// Copies the value into the stored value /// - void set(const T &value) noexcept(std::is_nothrow_assignable{}) + void set(const T &value) noexcept(std::is_nothrow_copy_constructible{} && std::is_nothrow_assignable{}) { - value_ = value; - has_value_ = true; + if (has_value_) + { + value_ref() = value; + } + else + { + new (&storage_) T(value); + has_value_ = true; + } + } + + /// + /// Moves the value into the stored value + /// + void set(T &&value) noexcept(std::is_nothrow_move_constructible{} && std::is_nothrow_move_assignable{}) + { + // note seperate overload for two reasons (as opposed to perfect forwarding) + // 1. have to deal with implicit conversions internally with perfect forwarding + // 2. have to deal with the noexcept specfiers for all the different variations + // overload is just far and away the simpler solution + if (has_value_) + { + value_ref() = std::move(value); + } + else + { + new (&storage_) T(std::move(value)); + has_value_ = true; + } + } + + /// + /// Assignment operator overload. Equivalent to setting the value using optional::set. + /// + optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) + { + set(rhs); + return *this; + } + + /// + /// Assignment operator overload. Equivalent to setting the value using optional::set. + /// + optional &operator=(T &&rhs) noexcept(noexcept(set(std::declval()))) + { + set(std::move(rhs)); + return *this; + } + + /// + /// After this is called, is_set() will return false until a new value is provided. + /// + void clear() noexcept(std::is_nothrow_destructible{}) + { + if (has_value_) + { + reinterpret_cast(&storage_)->~T(); + } + has_value_ = false; } /// @@ -83,7 +221,7 @@ public: throw invalid_attribute(); } - return value_; + return value_ref(); } /// @@ -97,26 +235,7 @@ public: throw invalid_attribute(); } - return value_; - } - - /// - /// Resets the internal value using its default constructor. After this is - /// called, is_set() will return false until a new value is provided. - /// - void clear() noexcept(std::is_nothrow_assignable{} && std::is_nothrow_default_constructible{}) - { - value_ = T(); - has_value_ = false; - } - - /// - /// Assignment operator. Equivalent to setting the value using optional::set. - /// - optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) - { - set(rhs); - return *this; + return value_ref(); } /// @@ -134,7 +253,7 @@ public: { return true; } - return value_ == other.value_; + return value_ref() == other.value_ref(); } /// @@ -148,8 +267,19 @@ public: } private: + // helpers for getting a T out of storage + T & value_ref() + { + return *reinterpret_cast(&storage_); + } + + const T &value_ref() const + { + return *reinterpret_cast(&storage_); + } + bool has_value_; - T value_; + typename std::aligned_storage::type storage_; }; } // namespace xlnt diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp index 7af72a20..1b32af87 100644 --- a/tests/utils/optional_tests.cpp +++ b/tests/utils/optional_tests.cpp @@ -109,8 +109,8 @@ public: xlnt::optional opt3(test_val); xlnt_assert(opt3.is_set()); xlnt_assert_equals(opt3.get().val, test_val); - //// no default ctor - //xlnt::optional no_def_opt; + // no default ctor + xlnt::optional no_def_opt; } void test_copy_ctor() From b004d0863c64d2fa0e6bada7bd6fe373baee393c Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Tue, 10 Jul 2018 17:26:34 +1200 Subject: [PATCH 34/38] address CI build failure Issue #300 Previous CI fix didn't work, try with type aliasing instead Issue #300 This reverts commit c87c0e39756062fbc761698e96978b0936005b0e. gcc 6 ok, msvc 14 choked Issue #300 noexcept(condition) doesn't seem to work on msvs 2015 Issue #300 Merge remote-tracking branch 'origin/dev-optional-no-default-ctor' into dev-resolve-simple-issues ensure uniqueness (uglify) and undefine compatibility macro --- include/xlnt/utils/optional.hpp | 53 ++++++++++++++++++++++++--------- tests/utils/optional_tests.cpp | 3 +- 2 files changed, 40 insertions(+), 16 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index 75fd4ac5..bc02407d 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -25,6 +25,7 @@ #include #include +#include namespace xlnt { @@ -36,6 +37,26 @@ namespace xlnt { template class optional { +#if _MSC_VER <= 1900 // v14, visual studio 2015 +#define XLNT_NOEXCEPT_VALUE_COMPAT(...) (false) + using ctor_copy_T_noexcept = std::false_type; + using ctor_move_T_noexcept = std::false_type; + using copy_ctor_noexcept = ctor_copy_T_noexcept; + using move_ctor_noexcept = ctor_move_T_noexcept; + using set_copy_noexcept_t = std::false_type; + using set_move_noexcept_t = std::false_type; + using clear_noexcept_t = std::false_type; +#else +#define XLNT_NOEXCEPT_VALUE_COMPAT(...) (__VA_ARGS__) + using ctor_copy_T_noexcept = typename std::conditional{}, std::true_type, std::false_type>::type; + using ctor_move_T_noexcept = typename std::conditional{}, std::true_type, std::false_type>::type; + using copy_ctor_noexcept = ctor_copy_T_noexcept; + using move_ctor_noexcept = ctor_move_T_noexcept; + using set_copy_noexcept_t = typename std::conditional{} && std::is_nothrow_assignable{}, std::true_type, std::false_type>::type; + using set_move_noexcept_t = typename std::conditional{} && std::is_nothrow_move_assignable{}, std::true_type, std::false_type>::type; + using clear_noexcept_t = typename std::conditional{}, std::true_type, std::false_type>::type; +#endif + public: /// /// Default contructor. is_set() will be false initially. @@ -49,7 +70,7 @@ public: /// Constructs this optional with a value. /// noexcept if T copy ctor is noexcept /// - optional(const T &value) noexcept(std::is_nothrow_copy_constructible{}) + optional(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(ctor_copy_T_noexcept{})) : has_value_(true) { new (&storage_) T(value); @@ -59,7 +80,7 @@ public: /// Constructs this optional with a value. /// noexcept if T move ctor is noexcept /// - optional(T &&value) noexcept(std::is_nothrow_move_constructible{}) + optional(T &&value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(ctor_move_T_noexcept{})) : has_value_(true) { new (&storage_) T(std::move(value)); @@ -69,7 +90,7 @@ public: /// Copy constructs this optional from other /// noexcept if T copy ctor is noexcept /// - optional(const optional& other) noexcept(std::is_nothrow_copy_constructible{}) + optional(const optional &other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(copy_ctor_noexcept{})) : has_value_(other.has_value_) { if (has_value_) @@ -82,7 +103,7 @@ public: /// Move constructs this optional from other. Clears the value from other if set /// noexcept if T move ctor is noexcept /// - optional(optional &&other) noexcept(std::is_nothrow_move_constructible{}) + optional(optional &&other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(move_ctor_noexcept{})) : has_value_(other.has_value_) { if (has_value_) @@ -96,7 +117,7 @@ public: /// Copy assignment of this optional from other /// noexcept if set and clear are noexcept for T& /// - optional &operator=(const optional &other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + optional &operator=(const optional &other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{} && clear_noexcept_t{})) { if (other.has_value_) { @@ -113,7 +134,7 @@ public: /// Move assignment of this optional from other /// noexcept if set and clear are noexcept for T&& /// - optional &operator=(optional &&other) noexcept(noexcept(set(std::declval())) && noexcept(clear())) + optional &operator=(optional &&other) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{} && clear_noexcept_t{})) { if (other.has_value_) { @@ -147,7 +168,7 @@ public: /// /// Copies the value into the stored value /// - void set(const T &value) noexcept(std::is_nothrow_copy_constructible{} && std::is_nothrow_assignable{}) + void set(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { if (has_value_) { @@ -163,7 +184,7 @@ public: /// /// Moves the value into the stored value /// - void set(T &&value) noexcept(std::is_nothrow_move_constructible{} && std::is_nothrow_move_assignable{}) + void set(T &&value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{})) { // note seperate overload for two reasons (as opposed to perfect forwarding) // 1. have to deal with implicit conversions internally with perfect forwarding @@ -183,7 +204,7 @@ public: /// /// Assignment operator overload. Equivalent to setting the value using optional::set. /// - optional &operator=(const T &rhs) noexcept(noexcept(set(std::declval()))) + optional &operator=(const T &rhs) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { set(rhs); return *this; @@ -192,7 +213,7 @@ public: /// /// Assignment operator overload. Equivalent to setting the value using optional::set. /// - optional &operator=(T &&rhs) noexcept(noexcept(set(std::declval()))) + optional &operator=(T &&rhs) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_move_noexcept_t{})) { set(std::move(rhs)); return *this; @@ -201,7 +222,7 @@ public: /// /// After this is called, is_set() will return false until a new value is provided. /// - void clear() noexcept(std::is_nothrow_destructible{}) + void clear() noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(clear_noexcept_t{})) { if (has_value_) { @@ -268,12 +289,12 @@ public: private: // helpers for getting a T out of storage - T & value_ref() + T &value_ref() noexcept { return *reinterpret_cast(&storage_); } - const T &value_ref() const + const T &value_ref() const noexcept { return *reinterpret_cast(&storage_); } @@ -282,4 +303,8 @@ private: typename std::aligned_storage::type storage_; }; -} // namespace xlnt +#ifdef XLNT_NOEXCEPT_VALUE_COMPAT +#undef XLNT_NOEXCEPT_VALUE_COMPAT +#endif + +} // namespace xlnt \ No newline at end of file diff --git a/tests/utils/optional_tests.cpp b/tests/utils/optional_tests.cpp index 1b32af87..6313e167 100644 --- a/tests/utils/optional_tests.cpp +++ b/tests/utils/optional_tests.cpp @@ -21,9 +21,8 @@ // @license: http://www.opensource.org/licenses/mit-license.php // @author: see AUTHORS file -#pragma once - #include +#include #include // test helpers From ad24d9485db9d9af7b2b3210c7601872fc1a6ba5 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Wed, 11 Jul 2018 12:27:18 +1200 Subject: [PATCH 35/38] Resolve CI warning about using an uninitialised variable --- include/xlnt/utils/optional.hpp | 6 +++ source/detail/serialization/xlsx_consumer.cpp | 42 ++++++++++--------- 2 files changed, 29 insertions(+), 19 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index bc02407d..e019f316 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -170,6 +170,8 @@ public: /// void set(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" if (has_value_) { value_ref() = value; @@ -179,6 +181,7 @@ public: new (&storage_) T(value); has_value_ = true; } +#pragma GCC diagnostic pop } /// @@ -190,6 +193,8 @@ public: // 1. have to deal with implicit conversions internally with perfect forwarding // 2. have to deal with the noexcept specfiers for all the different variations // overload is just far and away the simpler solution +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" if (has_value_) { value_ref() = std::move(value); @@ -199,6 +204,7 @@ public: new (&storage_) T(std::move(value)); has_value_ = true; } +#pragma GCC diagnostic pop } /// diff --git a/source/detail/serialization/xlsx_consumer.cpp b/source/detail/serialization/xlsx_consumer.cpp index 336ddaa3..a634bf7b 100644 --- a/source/detail/serialization/xlsx_consumer.cpp +++ b/source/detail/serialization/xlsx_consumer.cpp @@ -387,7 +387,7 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) props.sync_horizontal.set(parser().attribute("syncHorizontal")); } if (parser().attribute_present("syncVertical")) - {// optional, boolean, false + { // optional, boolean, false props.sync_vertical.set(parser().attribute("syncVertical")); } if (parser().attribute_present("syncRef")) @@ -399,11 +399,11 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) props.transition_evaluation.set(parser().attribute("transitionEvaluation")); } if (parser().attribute_present("transitionEntry")) - {// optional, boolean, false + { // optional, boolean, false props.transition_entry.set(parser().attribute("transitionEntry")); } if (parser().attribute_present("published")) - {// optional, boolean, true + { // optional, boolean, true props.published.set(parser().attribute("published")); } if (parser().attribute_present("codeName")) @@ -411,11 +411,11 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) props.code_name.set(parser().attribute("codeName")); } if (parser().attribute_present("filterMode")) - {// optional, boolean, false + { // optional, boolean, false props.filter_mode.set(parser().attribute("filterMode")); } if (parser().attribute_present("enableFormatConditionsCalculation")) - {// optional, boolean, true + { // optional, boolean, true props.enable_format_condition_calculation.set(parser().attribute("enableFormatConditionsCalculation")); } ws.d_->sheet_properties_.set(props); @@ -606,19 +606,22 @@ std::string xlsx_consumer::read_worksheet_begin(const std::string &rel_id) auto min = static_cast(std::stoull(parser().attribute("min"))); auto max = static_cast(std::stoull(parser().attribute("max"))); - optional width; - - if (parser().attribute_present("width")) - { - width = (parser().attribute("width") * 7 - 5) / 7; - } - - optional column_style; - - if (parser().attribute_present("style")) - { - column_style = parser().attribute("style"); - } + // avoid uninitialised warnings in GCC by using a lambda to make the conditional initialisation + optional width = [](xml::parser &p) -> xlnt::optional { + if (p.attribute_present("width")) + { + return (p.attribute("width") * 7 - 5) / 7; + } + return xlnt::optional(); + }(parser()); + // avoid uninitialised warnings in GCC by using a lambda to make the conditional initialisation + optional column_style = [](xml::parser &p) -> xlnt::optional { + if (p.attribute_present("style")) + { + return p.attribute("style"); + } + return xlnt::optional(); + }(parser()); auto custom = parser().attribute_present("customWidth") ? is_true(parser().attribute("customWidth")) @@ -1843,7 +1846,8 @@ void xlsx_consumer::read_office_document(const std::string &content_type) // CT_ target_.d_->sheet_title_rel_id_map_.end(), [&](const std::pair &p) { return p.second == worksheet_rel.id(); - })->first; + }) + ->first; auto id = sheet_title_id_map_[title]; auto index = sheet_title_index_map_[title]; From fede2d31680b9ddf218ccae1c4129ee29923d587 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 21 Jul 2018 12:33:58 +1200 Subject: [PATCH 36/38] fix Clang CI warnings --- include/xlnt/utils/optional.hpp | 9 +-------- source/CMakeLists.txt | 1 + 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index e019f316..f19bc286 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -37,15 +37,8 @@ namespace xlnt { template class optional { -#if _MSC_VER <= 1900 // v14, visual studio 2015 +#if defined(_MSC_VER) && _MSC_VER <= 1900 // v14, visual studio 2015 #define XLNT_NOEXCEPT_VALUE_COMPAT(...) (false) - using ctor_copy_T_noexcept = std::false_type; - using ctor_move_T_noexcept = std::false_type; - using copy_ctor_noexcept = ctor_copy_T_noexcept; - using move_ctor_noexcept = ctor_move_T_noexcept; - using set_copy_noexcept_t = std::false_type; - using set_move_noexcept_t = std::false_type; - using clear_noexcept_t = std::false_type; #else #define XLNT_NOEXCEPT_VALUE_COMPAT(...) (__VA_ARGS__) using ctor_copy_T_noexcept = typename std::conditional{}, std::true_type, std::false_type>::type; diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 2ea048d3..11e3593c 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -41,6 +41,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-padded") # ignore padding warnings set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-documentation-unknown-command") # ignore unknown commands in Javadoc-style comments set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas") # ignore Windows and GCC pragmas + set CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-warning-option") # ignore Windows and GCC pragmas set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-float-equal") # don't warn on uses of == for fp types set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-newline-eof") # no longer an issue with post-c++11 standards which mandate include add a newline if neccesary set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-covered-switch-default") # default is often added to switches for completeness or to cover future alternatives From 6562c41ae1e16f4bdfcfab5c48c8523c7dba5480 Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 21 Jul 2018 12:38:33 +1200 Subject: [PATCH 37/38] fix typo in Cmake lists --- source/CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt index 11e3593c..5c924e84 100644 --- a/source/CMakeLists.txt +++ b/source/CMakeLists.txt @@ -41,7 +41,7 @@ elseif(CMAKE_CXX_COMPILER_ID MATCHES "Clang") set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-padded") # ignore padding warnings set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-documentation-unknown-command") # ignore unknown commands in Javadoc-style comments set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-pragmas") # ignore Windows and GCC pragmas - set CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-warning-option") # ignore Windows and GCC pragmas + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unknown-warning-option") # ignore Windows and GCC pragmas set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-float-equal") # don't warn on uses of == for fp types set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-newline-eof") # no longer an issue with post-c++11 standards which mandate include add a newline if neccesary set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-covered-switch-default") # default is often added to switches for completeness or to cover future alternatives From 9e981abe0549a458dc918fe1cdb75532473ba22e Mon Sep 17 00:00:00 2001 From: Crzyrndm Date: Sat, 21 Jul 2018 13:30:52 +1200 Subject: [PATCH 38/38] Only add diagnostics to GCC, Clang doesn't know the warning --- include/xlnt/utils/optional.hpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/xlnt/utils/optional.hpp b/include/xlnt/utils/optional.hpp index f19bc286..8248ebd2 100644 --- a/include/xlnt/utils/optional.hpp +++ b/include/xlnt/utils/optional.hpp @@ -163,8 +163,10 @@ public: /// void set(const T &value) noexcept(XLNT_NOEXCEPT_VALUE_COMPAT(set_copy_noexcept_t{})) { +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif if (has_value_) { value_ref() = value; @@ -174,7 +176,9 @@ public: new (&storage_) T(value); has_value_ = true; } +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic pop +#endif } /// @@ -186,8 +190,10 @@ public: // 1. have to deal with implicit conversions internally with perfect forwarding // 2. have to deal with the noexcept specfiers for all the different variations // overload is just far and away the simpler solution +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif if (has_value_) { value_ref() = std::move(value); @@ -197,7 +203,9 @@ public: new (&storage_) T(std::move(value)); has_value_ = true; } +#if defined(__GNUC__) && !defined(__clang__) #pragma GCC diagnostic pop +#endif } ///