xlnt icon indicating copy to clipboard operation
xlnt copied to clipboard

const-ness propagation

Open Crzyrndm opened this issue 6 years ago • 1 comments

The const-ness of parent objects is being ignored

Expected behaviour: https://godbolt.org/g/huR6Kg A const collection can't be modified through naive iteration, compiler error if this is attempted

Current behaviour:

        xlnt::workbook wb;
        auto ws = wb.active_sheet();
        ws.cell("B3").value("B3");
        const auto ws_const1 = ws;
        auto cell = ws_const1.cell("B3");
        cell.value("other");
        assert(ws.cell("B3").value<std::string>() == "other"); // true

The core of this is that worksheets and cells are simple reference types. A worksheet "copy" doesn't actually copy anything. Some typical design options for reference types to prevent this are:

  • disable modifications (see std::string_view, const_iterator's)
  • delete copy ctor/assignment (move only types)
  • use templates to provide modification only when constructed from a modifiable instance (see gsl::span)

Crzyrndm avatar Jun 08 '18 23:06 Crzyrndm

I can't see a way to resolve this without a source breaking change to the library. On the flip side, it's likely that some users have accidentally made use of the bug without being aware of it.

Options as I see it:

  1. Making the types owners instead of references is a huge change that is likely to silently break pretty much every existing dependant project, and will create an ongoing list of bug reports. Additionally, it makes sense that a worksheet is always owned by a workbook and copy semantics would make that awkward (does it make silently make a copy in the parent? What happens when that instance is destructed? etc., etc.)
  2. Disabling modifications is out. There's no point in a read-only library
  3. Type duplication will work, but is generally a PITA. It would also be a significant design change in this library to have a core component be template based (the most reliable way to create types with mirrored behaviour). There are other ways to achieve it, but keeping them in sync will always be an issue
  4. Disabling the special member function copy and move operations (actual copying of the sheet/cell probably only through dedicated functions) will stop every current working usage from compiling (since these types all currently have to be used as values, ie. copies). However, the fix would simple in most of the usages (auto -> auto&) and a compile time break is much better than the silent breakage that would ensue with a change to non-reference types

It's probably clear that my vote is for option#4 and that is because I think that in many ways the current design is sensible. Lack of const propagation is only an oversight in the implementation. To resolve this through option#4 I can see three milestones

  1. Enabling references to affected types. Has a few other benefits so is a good development goal anyway. Additionally, see what warnings / messages can be enabled to push users towards taking references instead of copies to minimise later work.
  2. Creating new types for all affected classes (This can be done in a v2 (naming debatable) namespace or through new class names. Presence of inline namespaces which are intended for versioning and the obvious names currently in use would indicate that namespacing is the least surprising solution). During this phase, these are experimental classes.
  3. Elevate the new classes to primary status, deprecate affected existing classes (probably use a macro to allow old behaviour to be retained as required). Semantic versioning would require this to occur with a new major version since it is breaking. Once stage 2 is complete, there is no major rush to push this through (could recommend users to add a define in order to transition early while other release components are established).

@tfussell Impressions? Would a different option be preferable (or one I haven't thought of)?

Crzyrndm avatar Jun 17 '18 11:06 Crzyrndm