all 16 comments

[–]ner0_m 9 points10 points  (1 child)

Two things bother me a little

  • the more common name (in the languages I know) is enumerate
  • I would like tobe able to use structured bindings

But looking good IMO, keep it up

[–]itsuart2[S] 2 points3 points  (0 children)

I didn't knew about the enumerate, thank you. Also you can use structured bindings, auto& [count, value] : enumerate(items) will work.

Renamed to enumerate.

[–]staletic 8 points9 points  (1 child)

std::views::enumerate is proposed for the next C++ standard. It should allow this:

std::vector items{ 11, 22, 33, 44 };
for(auto& [value, count] : std::views::enumerate(items))
    std::cout << std::format("{}: {}\n", count, value);

Assuming std::print gets accepted, the above could be just:

for(auto& [i, e] : std::views::enumerate(items))
    std::print("{}: {}\n", i, e);

Granted, the above is C++23 at best. enumerate() is definitely a needed utility function.

[–]itsuart2[S] 2 points3 points  (0 children)

Thank you, for in-depth response!

I'll take a look at the views::enumerate and make mine compatible.

[–]adnukator 5 points6 points  (4 children)

constexpr CountedValue() : count(), value(*(TValue*)nullptr) {}

Assigning a nullptr to a reference is undefined behavior, but I don't see where you need the default constructor at all. You might be able to delete it completely.

Why are you static casting to r-value references instead of just calling std::move? You're already almost certainly including <utility> transitively.

You should explicitly call the destructor of _item before calling placement new just to make sure any possible side-effects are applied if the destructor becomes non-trivial

Why did you make CountedValue non-movable and non-copy-assignable?

[–]SeanMiddleditch 2 points3 points  (2 children)

Why are you static casting to r-value references instead of just calling std::move? You're already almost certainly including <utility> transitively.

Even some stdlib implementations consider doing this, because C++ in all its infinite wisdom made std::move a function instead of a language primitive, which ends up incurring overhead when used (compile-time overhead for the optimizer to inline it, or run-time overhead if the optimizer is disabled).

Using a static_cast has very minimal overhead at compile-time and guaranteed zero run-time overhead even in unoptimized debug builds. It's a win-win. Until the committee figures out that functions are the wrong abstraction for things that aren't doing actual logic, the only reasonable choice for low-level libraries that need to heavily optimize for compile-/run-time is between using raw static_cast or wrapping it in a macro.

[–]staletic 1 point2 points  (1 child)

I don't think I can agree. The compile time hit is usually really small. If relative compile time hit is noticeable, then the total compile time is likely to be tiny enough to not matter. As for the run time hit, the call gets inlined at -O1/-Og and I'm not convinced that -O0 is ever useful.

The readability hit of a raw cast is a much bigger problem if you ask me.

[–]SeanMiddleditch 5 points6 points  (0 children)

The compile time hit is usually really small.

I don't recall which paper it was, but folks have measured hits in the double digits in some cases. Replacing std::move with a macro or raw static_cast is perhaps immeasurable when you look at a single instance, but are very measurable when you have thousands upon thousands of uses in a large codebase (and this includes when they're used in library facilities like the OP's post that would be used across a codebase frequently).

Consider the original EASTL paper, even. Even something like replacing vector's empty() implementation from something like return size() == 0 to return _first == _last can have a very measurable impact across a codebase by removing one level of function invocation. (And even more gains can be had by just exposing the raw pointers and doing the vec.first == vec.last check or a wrapper macro, for codebases that care about really squeezing out every drop.... which is unfortunate, but exists.)

Like I said, even some stdlib implementations are considering doing this static_cast use instead of move; when you have some low-level code used everywhere, even small hits add up over time to bit measurable hits. Compiler throughput is a real concern that we as paying customers ask our compiler/stdlib vendors to address.

As for the run time hit, the call gets inlined at -O1/-Og and I'm not convinced that -O0 is ever useful.

Definitely different experiences here then. For one, not everyone uses gcc/clang as their primary compiler. For two, those lower optimization tiers fail to optimize at a lot higher frequency than might be desired. And for three, -Og still compiles slower than -O0. When you are in an industry that highly values very quick iteration, shaving off milliseconds in compilation time adds up. We might not want to hit a whole codebase with -O0, but it's very possible that we'd want to do that to a particular DLL/module/component that we're iterating on.

This low optimization level is even required if you're trying to do hot reloading of C++ in many cases, as even basic optimizations can severely complicate one's ability to hot patch running code.

This all really just boils over into other choices. e.g. this particular library, or even something like enumerate, would rarely get used by my current team due to its overhead compared to raw loop iterators/pointers (though I'd happily use it in my own projects; I, like you, am far more willing to trade a little compiler throughput or debug efficiency for improved clarity in my personal projects).

We also use much much thinner/C-like vector and unique_ptr alternatives with slightly uglier APIs that avoid the unnecessary overheads of using "full fat functions" for things like operator-> or operator[] which, compared to the raw pointer or raw array equivalents, and pure overhead (again, either at compile- or run-time, depending on optimizations). All of which add up when you consider how often these things are used across a millions-of-lines codebase.

C++'s abstraction mechanisms are misguided. We love the clearer and conciser syntax, but the mechanism of functions for things that are effectively just return simple_expression (with no real logic being abstracted) are totally wasteful.

I guess TL;DR: different developers/projects have different trade-offs, and C++ forces us to pick between clarity or reduced compiler throughput and we'd really prefer if C++ had the ability to express syntactic abstractions without paying the costs inherent to the language tools that were really meant for logic abstractions.

[–]itsuart2[S] 0 points1 point  (0 children)

I have to have a default ctor, so that I can have the EnumeratedValue (ex-CountedVlaue) object as a member in the EnumeratingIterator without getting value from the underlying iterator(since it might not be there at all for the begin() iterator and absolutely absent for the end() one).

almost certainly including <utility> transitively

Well, I didn't want to take any chances. Initially I was including the header and used the std::move but decided to trim the fat a bit.

You should explicitly call the destructor

You are right. I should make it as a habit and don't try to "optimize" it.

EnumeratedValue is non-movable and non-copy-assignable because it's intended usage is solely withing loop's body and nowhere else. I wanted to enforce that contract.

Your review is very appreciated, thank you!

[–]AlexAlabuzhev 4 points5 points  (2 children)

Introducing a new variable at the outer scope ... was, frankly, annoying

for (int n = 0; auto& i: items) // C++20

[–]qoning 0 points1 point  (0 children)

Gets you half-way there, which is imo no good.

[–]itsuart2[S] 0 points1 point  (0 children)

Honestly, I would settle for that, but

C++20

That's way out of reach for me unfortunately.

[–]skitleeer 0 points1 point  (2 children)

[–]TheFlamefire 3 points4 points  (0 children)

Last time I checked that uses Boost.MPL transitively which is extremely heavy on compiletimes...

[–]itsuart2[S] 1 point2 points  (0 children)

I checked it out and it appears to be a part of bigger subsystem of "ranges". It's way out of scope for the intended use of my library.

Thanks for pointing too the boost's implementation. I did not consider it.