A problematic iteration pattern

[aerrant]

While getting acquainted with the codebase, I've noticed the mess that is iteration over the various lists. It goes as follows: whenever you need to to inspect some subset of a list, you use a method that builds a container, populates it with the requested data, then returns it as an rvalue reference which you're expected to iterate over - again! It's goes deeper: some lads and ladies apparently missed their CS classes, so there are instances of returning sets and lists too. And even deeper: with the ban on auto, the types of those containers spread all over the code, making you guess which exact type you should expect, and also prompting you to use type aliases to further obfuscate the code. I don't even want to think of the performance implications of copying lists for the game that is all about lists.

Just take a look at how delightfully diverse the signatures from vechicle.h alone are:
const std::set<fault_id> &faults() const;
std::set<fault_id> faults_potential() const;
std::vector<itype_id> get_printable_fuel_types() const;
std::map<itype_id, int> fuels_left() const;

Admittedly, my experience with C++ is stuck at '03 level, but really, shouldn't there be a convention for that? I can think of five-ish possible conventions each with its pros and cons, but I'd like to hear from you guys first. Am I reading too much into it?

[kevingranade]

Some examples of this occurring would have made this potentially helpful.

some lads and ladies apparently missed their CS classes,

And someone apparently forgot to check their attitude before casting shade at potential collaborators. Nobody's perfect I guess.

don't even want to think of the performance implications

Don't think about the performance implications, profile it and find out how many cycles it's using. The vast majority of this kind of thing is a hypothetical cost, either because the data structure is tiny or because it's on a cold path where it doesn't matter. If you can point a static analysis tool that will fix it for you it's worth polishing up the codebase a bit, but when you're proposing someone manually auditing the codebase for this kind of thing, it's an incredibly low-margin task.

[esotericist]

It goes as follows: whenever you need to to inspect some subset of a list, you use a method that builds a container, populates it with the requested data, then returns it as an rvalue reference which you're expected to iterate over - again!

so you are reporting that things that need to examine a collection must be given a collection to examine? i'm not sure the problem you are perceiving here.

It's goes deeper: some lads and ladies apparently missed their CS classes, so there are instances of returning sets and lists too.

what kevin kindly sidesteps in his response is that your presumption here is rather extravagant considering you do not know the actual education and practical experience of the team working on the project.

And even deeper: with the ban on auto, the types of those containers spread all over the code, making you guess which exact type you should expect, and also prompting you to use type aliases to further obfuscate the code.

a) there is no 'ban' on auto, we simply prefer using it when it isn't necessary. even the compiler can when prompted (as we do in some of our continuous integration tasks) complain if you use auto unnecessarily, because excessive use of auto is actually well understood at this point to obfuscate what you're dealing with.
b) fascinatingly, considering the topic at hand, iterators are in fact a place where they are often appropriate; if you look at how collections typically get used after being fetched by the appropriate source, they are generally using auto as the type for iterators.
c) without a concrete example i cannot fathom what you actually intend to convey with "prompting you to use type aliases to further obfuscate the code"

Just take a look at how delightfully diverse the signatures from vechicle.h alone are:
const std::set<fault_id> &faults() const;
std::set<fault_id> faults_potential() const;
std::vector<itype_id> get_printable_fuel_types() const;
std::map<itype_id, int> fuels_left() const;

so you are reporting that methods which need to return objects of different types with different storage and access patterns happen to return objects of different types inside collections with different storage and access properties? again, i do not see the issue that you appear to see.

Admittedly, my experience with C++ is stuck at '03 level, but really, shouldn't there be a convention for that?

so you are reporting that you are not that familiar with how c++ has been written in the last nearly two decades, but that you have opinions in how it should be written regardless? i struggle to see why this warrants the above.

[actual-nh]

It's goes deeper: some lads and ladies apparently missed their CS classes, so there are instances of returning sets and lists too.

Was this actually meant to imply lack of knowledge, or a liking for using more-advanced techniques (taught in said classes)? I am hoping it was the latter.

[mlangsdorf]

My C++ classes predate large parts of the C++ standard library, but I'm not sure what the issue is with returning sets and lists. Is the issue that returning them creates a copy and they should be returned by reference?

[anothersimulacrum]

I'm fairly sure in most of the places that we return copies, it's because we're returning a subset of the list, and we don't really have many other options, and of the few I can think of aren't really any better.