Remix.run Logo
kiitos 9 days ago

> I was specifically inspired by a performance bug due to a typo. This mistake is the “value param” vs “reference param” where your function copies a value instead of passing it by reference because an ampersand (&) was missing ... This simple typo is easy to miss

the difference between `const Data& d` and `const Data d` isn't accurately characterized as "a typo" -- it's a semantically significant difference in intent, core to the language, critical to behavior and outcome

even if the author "forgot" to add the `&` due to a typo, that mistake should absolutely have been caught by linting, tests, CI, or code review, well before it entered the code base

so not feelin' it, sorry

eptcyka 5 days ago | parent | next [-]

If the implications of a one char diff are this egregious that they’re considered obvious, maybe it should take less cognitive effort to spot this? CI and tooling are great, but would be far less necessary if it was more difficult to make this mistake in the first place.

kiitos 16 hours ago | parent | next [-]

if you are programming in C++ then you have opted-in to a set of syntax and semantic properties that are ancient and well-defined and core to the language. those properties include at a very basic level exactly the sigil under discussion here.

it is not productive or interesting to characterize this absolutely core property of the language as "a one char diff" that takes any kind of special cognitive effort to spot

Disposal8433 5 days ago | parent | prev [-]

What do you suggest? Some kind of std::const_reference<Type>? Clang-tidy is enough in addition to the reviews.

eptcyka 5 days ago | parent | next [-]

The person is arguing that it is a massive difference, not a typo. I am saying that if that is the case, then maybe the hamming distance between correct and buggy code that both compile should be greater than 1, regardless if more tooling can help solve the problem or not.

I specifically take issue with this framing of it is not an issue for we have the tools to help with this, especially where the tools are not part of a standard distribution of a toolchain and require more than minimal effort. C++ has had many a warts for many decades, and the response has always been *you are just holding it wrong* and not running a well covering integration test suite with sanitizers on every commit, you just need to run one more tool in the CI, just a comprehensive benchmarking suite, have more eyes looking for a single char difference in reviews.

Mesopropithecus 5 days ago | parent | prev | next [-]

I'm seeing this way too often in production code, despite linters and reviews. So we have to keep plastering over.

qalmakka 4 days ago | parent | prev [-]

The problem is not the reference, the problem is implicit copies and the horses left the barn 40 years ago, it's too late to fix that. The only thing we can do right now is deleting or marking copy constructors explicit whenever possible

lock1 5 days ago | parent | prev | next [-]

Disclaimer: I didn't have any production experience, only side projects in both C++ & Rust.

I think the problem with `T &d` and `T d` is that these 2 declarations yield a "name" `d` that you can operate on very similarly. It's not necessarily about reference declaration `T& d` is 1 char diff away compared to value declaration `T d`.

While there is a significant semantic difference between declaring things as a value and as a reference (&), non-static member function invocation syntax is the same on both `&d` and `d`. You can't tell the difference without reading the original declaration, and the compiler will happily accept it.

Contrast this to `T *d` or `T d`. Raw pointers require different operations on `d` (deref, -> operator, etc). You're forced to update the code if you change the declaration because the compiler will loudly complain about it.

It shares the same problem with a type system with nullable-by-default reference type vs an explicit container of [0..1] element Option<T>. Migrating existing code to Option<>-type will cause the compiler to throw a ton of explicit errors, and it will become a breaking change if it was a public API declaration. On the other hand, you're never able to feel safe in nullable-by-default; a public API might claim it never return `null` in the documentation, but you will never know if it's true or not only from the type signature.

Whether it's good or bad, I guess it depends on the language designer's decision. It is certainly more of a hassle to break & fix everything when updating the declaration, but it also can be a silent footgun as well.

Dylan16807 5 days ago | parent | prev | next [-]

It's const so you're not changing it, and you're not sneaking a pointer either. So what's the difference in intent?

dzaima 4 days ago | parent | prev | next [-]

Problem is it doesn't affect outcome at all unless you do mutation, and as such testing is irrelevant, but still can significantly impacts perf, and performance problems can take a while to surface; like, it may slowly grow from 0.1% of runtime to like 2%, low enough to not get get noticed at all at first, and still be too low to have significant thought put into it afterwards (but still way too high from a single missing character).

And, as you said, this is a meaningful difference in intent, so linting can't just blanket complain on every single instance of a non-&-ed argument.

And the difference in writing down intent is the wrong direction - doing a full nested object clone should require adding code in any sane language, whereas, in C++, making code clone takes.. negative one characters.

Whereas in Rust, the only thing that's ever implicit is a bitwise copy on objects with constant size; everything else requires either adding &-s or .clone()s, or your code won't compile.

cozzyd 9 days ago | parent | prev | next [-]

yeah, I assumed this was going to be some sort of 100 screens of template error nonsense, not an obvious mistake (that is also trivial to find while profiling)

qalmakka 4 days ago | parent | prev [-]

The fact that implicit copies are a feature doesn't mean they were a good design choice to begin with. In new code I've started making the copy constructor explicit whenever I can, for instance, just to avoid this kind of shenanigans