Skip to content

Robustness/minor: optional copy-assignment leak, iterator type mismatch, comparator dropped in lazy zip #31

@jkalias

Description

@jkalias

Summary

Lower-severity robustness and consistency issues found during review.


1. (C++11/14 fallback) optional::operator= leaks the previously held value

Location: include/optional.h:55-62

optional& operator=(optional const& other)
{
    _value = nullptr;                          // leaks existing _value if non-null
    if (other.has_value()) {
        _value = new T{other.value()};
    }
    return *this;
}

Copy-assigning to an optional that already holds a value overwrites _value without deleteing the previous allocation → memory leak. The value-assignment overload (operator=(T const&), line 100) correctly calls reset() first; this one does not.

This affects only the pre-C++17 fallback path (C++17 uses std::optional).

Fix:

optional& operator=(optional const& other)
{
    if (this == &other) {
        return *this;
    }
    reset();
    if (other.has_value()) {
        _value = new T{other.value()};
    }
    return *this;
}

A self-assignment guard is also worth adding. (Consider the same review for the copy constructor's exception safety.)


2. set iterator accessors use the default-comparator iterator type

Location: include/set.h:1076-1097 (begin/end) and zip_impl (1187)

[[nodiscard]] typename std::set<TKey>::iterator begin()    // should be std::set<TKey, TCompare>

The member is std::set<TKey, TCompare> but the iterator types are spelled std::set<TKey>::iterator. This compiles today because iterator types are independent of the comparator in practice, but it is not guaranteed by the standard and is fragile. Spell them std::set<TKey, TCompare>::iterator (or use decltype(m_set)::iterator).


3. lazy_set::zip(vector) drops the comparator

Location: include/set.h:362-376

std::set<UKey> distinct_values(vector.begin(), vector.end());   // default std::less<UKey>

The deduplication set is built with the default comparator, ignoring UCompare. Minor, but inconsistent with the rest of the comparator-preserving design. The non-lazy set::zip(vector) path (which routes through vector::distinct) is also worth checking for the same behavior.


4. Consistency: [[nodiscard]] and parameter passing

Minor polish opportunities noticed while reading:

  • insert_back / inserting_back take the single element by value T (see the performance issue for the move fix).
  • Some non-mutating returns could be [[nodiscard]] for parity with their siblings.

These are cosmetic; grouping here so they aren't lost.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions