Unconditional exit action

[u/Shevvv]

Recently I started a C++ project, and while I do have some C experience, this is a new language for me, so I have doubts about a lot of the paradigms that ChatGPT swears are the way to go. Just to be clear, I write all of my code myself, architecture and implementation, and use ChatGPT as a consultant/reviewer.

I currently have the following model: a BooksReport class that stores book piles sorted by size within three groups: Uniform, Nuniform and Singles. trying to traverse a single group while popping piles at the same time was quite cumbersome, because popping invalidates iterators and there are several cases when the end of a size of piles is reached, or when the size has become empty, or the end of the group is reached... So I embedded a private Cursor class that allows me to traverse a single group within BooksReport the following way:

for (
    booksReport.resetCursor(BooksReport::Group::Singles);
    booksReport.cursorIsValid();
    booksReport.advanceCursor()
) {
    auto [size, name] = booksReport.readCursor();

    if (iWantThisPilePopped(size, name))
        booksReport.popCursor();
}

When popCursor() is called, Cursor is alerted of an incoming pop, to which it responds by recalculating the indices to advance to during the next advanceCursor() call. Current implementation allows no more than one popping per loop, which I hope is a fair assumption to make during a traversal. Also, as you can see, a Cursor is either valid (which it becomes upon resetCursor() or invalid (it becomes invalid by reaching the end of the group). The valid attribute not only controls the loop, if it's set to false, it also block all Cursor-related operations, such as popCursor()

However, sometimes I exit the loop prematurely. Sometimes it's a break, sometimes I return from inside the loop. Technically, I end up with a Cursor that is valid outside of the loop, which is not ideal, since then I can popCursor(), which is not the intended use. ChatGPT offers the following solution:

#include <scope>

{
    auto onExit = std::scope_exit([&] {
        booksReport.clearCursor();
    });

    for (booksReport.resetCursor(BooksReport::Group::Singles);
         booksReport.cursorIsValid();
         booksReport.advanceCursor()) {

        if (something)
            break;

        if (somethingElse)
            return tasks;

        if (bad)
            throw std::runtime_error("bad");
    }
}

Is this a common paradigm? Does my situation warrant this? For now it's just a personal project. I intend to make it open source once it's finished (if anyone will see it as valuable). In an ideal world I'd add plug-in support for others, where other developers can use a limited number of API calls, including the public members of BooksReport, but I'm already tired from this side project that I'm not sure it will come to this.

EDIT: thank you all very much for the answers! As much as I'd like to move on to the next part of the project, refactoring this to be a separate object with the lifetime tied to the loop itself is indeed the most logical and simple solution!

One of these days, I'll reach the production state 😅

[u/IyeOnline]

I would strongly advise against this. You are designing an API from zero, so your design has the chance of being safe by construction. If your cursor is an internal state of the BooksReport class itself, but still must be manually managed from the outside, something is wrong.

Instead, the cursor should actually be the object that contains the information, instead of holding this state into class BookReport itself.

One common paradigm here would be to have an accessor that returns a range (an iterable object), which holds both the information for iteration as well as the mutating API. This means that you cannot perform these mutating operations without having such an object and that any "cleanup" automatically happens when the object is destroyed.

[u/Shevvv → u/IyeOnline]

It does sound like a better idea! I suppose the only thing I'd be losing by going over from a private cursor to an external cursor is that the current implementation also catches calls to BooksReport::popFront() and adjusts its position accordingly, and it's also robust in regards to a not-yet-implemented-if-ever BooksReport::pop(randomPosition). Technically, you aren't expected to use popFront() when iterating using a Cursor. But then again you're not expected to popCursor() when not within a loop, which was why I made the post.

[u/IyeOnline → u/Shevvv]

You can still implement push/pop with such an iterator pattern.

However, a big reason why erase on all standard library contains takes and returns an iterator is to make this at least partially safe by design. You cannot erase an object without an explicit iterator and doing an erase gives you back a new iterator to continue your iteration.

An alternative solution is to have the erase() on the iterator itself and then mutating that iterator. This is slightly safer (because a user cant forget to use the return value), but an Container::erase(Iterator) has other use-cases.

Crucially both solutions force users to use iterators explicitly (if they want to erase), so you cant e.g. break a range-based for loop as easy.

The other neat thing is that you can still have your range-based for loops as long as nobody wants to mutate the structure during iteration.

[u/Shevvv → u/IyeOnline]

My original idea was indeed to have it all iterator based. But then I ran into a problem within my

std::map<size_t, std::vector<std::string>, std::greater<>>

through which I'm iterating. This obvsiously requires two iterators, one through the key-value pairs, and the other through the vector elements within a single key-value pair. If a certain std::vector<std::string> becomes empty, I have to delete the entire key-value entry, obtain the new map iterator and assign my vector iterator to the first element within the vector from the new key-value pair. These pair of adjusted iterators is returned by the pop()/erase() function.

The problem is when the pop()/erase() call produces an empty map. There's no way I can generate a dummy vector iterator in this case (or at least I failed to think of one), so I moved away from the iterator approach to an index approach. This way my cursor can first check the validity of the current/upcoming indices and then translates them to iterators if everything is in order. I do lose access to a lot of self-propagating iterator-based functions this way though.

[u/looncraz]

Iteration is a solved problem in C++ using iterators and auto range loops.

What you want is a standalone object that references the position in the container, this allows you to permit other threads to also iterate at the same time.

You rarely modify a list during iteration, that's dangerous, you can remove one element and return immediately or you can make a second list that contains references to the items in the first list that needs to be removed. The STL also has many facilities for handling that situation.

[u/Shevvv → u/looncraz]

Well, it becomes cumbersome when it's an iteration within a nested

std::map<size_t, std::vector<std::string>, std::std::greater<>>

that has to be treated as a linear sequence. Which is the whole idea behind the Cursor class - it hides the logic involved in iterating this nested structure in a way that makes it convenient.

But my question isn't about how to iterate. It's about how to tidy thing up upon exit from a loop, no matter how that exit occurs.

EDIT: Just saw your edit. I suppose having an external object with the lifetime within the scope of the loop is a solution I didn't think of. But this needs rewriting then. And then communication with popFront(), a member of BooksReport, becomes problematic.

[u/saf_e]

That's what scoped_exit for

[u/OldAd9280]

A loop through a vector which erases elements should be of the form:  for (auto it = vec.begin(); it != vec.end();) {   if (foo(*it)   {      it = vec.erase(it);   }   else   {     it++   } } For simple cases like this you can use std::erase_if https://en.cppreference.com/cpp/container/vector/erase2 or std::remove_if and vec.erase() if you don't have c++20 https://en.cppreference.com/cpp/algorithm/remove

[u/Shevvv → u/OldAd9280]

I do use this pattern for simple vectors. In this particular case, however, I'm using:

std::map<size_t, std::vector<std::string>, std::greater<>>

Which is very nice an intuitive for storage and retrieval, but is a bit cumbersome for iterating

[u/ZMeson → u/Shevvv]

You can safely erase elements inside a loop if you use std::unordered_map:

https://en.cppreference.com/cpp/container/unordered_map/erase

Removes specified elements from the container. The order of the remaining elements is preserved. (This makes it possible to erase individual elements while iterating through the container.)

EDIT: You can also use std::erase_if on an unordered_map: https://en.cppreference.com/cpp/container/unordered_map/erase_if

And you can also use std::erase_if on most standard containers including std::map: https://en.cppreference.com/cpp/container/map/erase_if

[u/Plenty-Midnight2075]

scope_exit itself is a perfectly reasonable RAII pattern, but I think here it would be fixing a symptom rather than the underlying design.

The part that feels odd to me is that the traversal state lives inside BooksReport. That means BooksReport effectively has one global cursor, which is why you now need to worry about resetting/clearing it and what state it is left in after break, return, exceptions, etc. It also makes nested or multiple simultaneous traversals awkward.

I'd make the cursor an actual object whose lifetime represents the traversal instead:

auto cursor = booksReport.cursor(BooksReport::Group::Singles);

while (cursor) {
    auto [size, name] = cursor.read();

    if (iWantThisPilePopped(size, name))
        cursor.pop();
    else
        cursor.advance();
}

Then cursor simply ceases to exist on break, return, or exception because normal C++ object lifetime/RAII handles that automatically. Ideally pop() would also leave the cursor pointing at the next valid element, so the caller doesn't have to know about iterator invalidation at all.

I'd also consider making pop() an operation on the cursor rather than BooksReport::popCursor(). Then it is literally impossible to call it without possessing an active traversal object.

So yes, a scope guard is a normal tool for "this action absolutely must happen when I leave this scope", especially when wrapping C APIs or cleanup that can't naturally be represented by an object. But in this particular case you already have something with a natural lifetime: the traversal itself. I'd model that lifetime directly rather than adding another cleanup mechanism around shared cursor state.

[u/Shevvv → u/Plenty-Midnight2075]

I also have popFront(). Do I also make it part of the cursor? Or do I just keep popping every loop without advancing the cursor at all?

Either way thank you for the answer!

[u/mredding]

book piles sorted by size within three groups: Uniform, Nuniform and Singles.

It took quite a bit to realize N wasn't a variable, but means Non. Spell it out. Think about the next guy. You don't code on punch cards, you don't need to spare character counts.

trying to traverse a single group while popping piles at the same time was quite cumbersome

Pain is intuition trying to tell you something. You should listen to it rather than ignore it. You're talking about doing two different things across two different container types. Stacks and queues don't have iterators because they break the push/pop idiom.

Pick one.

I embedded a private Cursor class that allows me to traverse a single group within BooksReport

It sounds like a container of containers.

If you want to traverse a container of containers, you want to flatten the container with a view. Consider std::views::join.

Current implementation allows no more than one popping per loop, which I hope is a fair assumption 

It is not. It sounds like a completely arbitrary limitation imposed for no reason.

I exit the loop prematurely. Sometimes it's a break

I don't ever recommend this one specifically. Yes, the language allows it, but it makes the loop invariant obscure or contradictory. Returning early makes perfect sense.

Is this a common paradigm?

onExit is an example of RAII, arguably the most fundamental idiom of C++. This is exactly how to solve the conundrum you cornered yourself in.

Does my situation warrant this?

I think your situation warrants a redesign that is simpler, smaller, and faster.

[u/AutoModerator]

Your posts seem to contain unformatted code. Please make sure to format your code otherwise your post may be removed.

If you wrote your post in the "new reddit" interface, please make sure to format your code blocks by putting four spaces before each line, as the backtick-based (```) code blocks do not work on old Reddit.

I am a bot, and this action was performed automatically. Please contact the moderators of this subreddit if you have any questions or concerns.

[u/montymole123]

What is the <scope> library? Where did you get this from?

[u/Shevvv → u/montymole123]

ChatGPT hallucinating, as it turns out. The class in question is from <experimental/scope>, all the more reason to avoid its usage.

[u/alfps]

❞ Is this a common paradigm

Not in C++. But an iterator provided directly by the collection is a thing in Eiffel. Or was, I just sort of looked at Eiffel some decades ago.

I know one use case for that in C++, namely for a buffer gap structure. You can move the gap (which is the point of this structure) and that's an insertion and deletion position so it's effectively an iterator in the structure.

As others have already explained, idiomatic C++ for deletion while iterating over items, is to update the iterator with the iterator value you get from erase or whatever deletion operation, in that case instead of ++it.

[u/mredding]

I should also add I realize you're likely using that enum as an ad-hoc type system - instead of using RTTI, you're storing an enum associated with a type, and that's deciding subsequent behavior, like casting or pointer use.

Instead, you should be using the type system you have:

struct uniform {};
struct non_uniform {};
struct singles{};

using book_report = std::variant<std::monostate, uniform, non_uniform, singles>;

This will give you much stronger type safety and make your code simpler.

[u/Shevvv → u/mredding]

Thanks for the reply! No, the use of that enum is slightly different. A single BookReport stores three sets of data:

``` public: enum class Group { Uniform, Nuniform, Singles };

private: using SizesMap = std::map<size_t, std::vector<std::string>, std::greater<>>; size_t total = 0; size_t uniformTotal = 0; SizesMap uniformSizes; size_t nuniformTotal = 0; SizesMap nuniformSizes; size_t singlesTotal = 0; SizesMap singlesSizes; std::pair<const size_t &, const SizesMap &> selectGroup(Group booksGroup) const; std::pair<size_t &, SizesMap &> selectGroup(Group booksGroup); ```

Using selectGroup() allows me to choose the right pair of size_t and SizesMap to work with within the algorithm.

EDIT: I actually do use std::variant<T, U> inside a custom class Pile which behaves like a folder within a tree structure that can contain either only folders (other Pile objects) or only Book objects. I have indeed an enum that basically is used for querying a Pile object if it's storing Pile, Book or Empty (the container within is empty and as such the Pile object accepts both other Pile objects and Book objects). But I'm not exactly sure how to rewrite it according to what you suggest, or whether I should because I spent a lot of time designing an API for that class that is convenient to use (for me; that class is not meant to ever be exposed for external plugins). But that's a very different module of the project not involved in the algorithm I'm currently writing.

[u/mredding → u/Shevvv]

size_t xTotal = 0;
SizesMap xSizes;

So then THIS is a type. It just needs a name and some structure. You repeat this 3x. Perhaps you should then write:

// This is a C idiom that might be useful here; since you're using the
// enum as an index of integers, the weaker C enum is more appropriate,
// so you're not casting to the underlying type all the time.
enum group_indexes { begin, uniform = begin, non_uniform, singles, end, count = end };

std::array<count, sizes_type> sizes;

Now you can index:

return sizes[uniform];

Or:

return sizes[0];

Or you can loop:

for(auto index = begin; index < end; fn(sizes[index++]));

And because the enum is compile-time, the compiler can unroll that loop.

Or you can jump:

switch(index) {
case uniform:
case non_uniform:
case singles:
default: break;
}

But I still suggest you make 3 DIFFERENT types, because while the data is structually the same, uniform vs. non_uniform vs. singles doesn't sound like this data can be interchanged - it would likely be an error that singles data winds up in non_uniform data, no? The best way to prevent that is to use the type system to make it tangibly impossible, reducing to a compiler error. There is still a semantic difference and the compiler can enforce that.

Curry-Howard correspondence tells us that there are similarities between coding and proof writing. Your statements are posits, your source code is a theory, the compiler is the solver, and the generated program is the proof. We want elegant proofs, which means we need elegant theories. By leveraging the type system, you can make elegant theories by building compounding expressiveness - simpler words that allow you to create more complex words and statements about the theory. The theory becomes self-referential and internally consistent. I mean - just read a proof, they invent a lot of their own little universe, and then demonstrate themselves in terms of it. We're doing the same thing.

As for the rest of the code that processes this data, because it's generic across 3 different types, you implement that in terms of templates. You write that code once and let the compiler generate the appropriate machine code.

This template code gives you great flexability - you can specialize a template around any one of these types, if there are unique or optimal considerations for that type. You can also consider how you might factor type-independent code OUT of the template to reduce object bloat, so the templates become thin wrappers that enforce type safety.

And if the underlying data can interchange, you can do so explicitly.