On 10/30/2017 10:03 PM, Kris Maglione wrote:
On Mon, Oct 30, 2017 at 08:28:39AM -0700, Jim Blandy wrote:
Okay, this is half the argument. The second half would be:
- Does auto cause such mistakes more often than it prevents them? The
benefit claimed for auto is that it usually makes code more legible.
Hopefully that prevents mistakes, on the balance.
My feeling is that these features generally prevent more errors than they cause.
My gut feeling while having my reviewer's hat on, is that I haven't really seen
them preventing issues, but they have definitely caused issues.
The case for `auto` probably isn't as strong as the cases for other features,
but I don't think the case against it is as strong either. And there are
places where, without `auto`, the extra qualified type noise spreads a simple
assignment across multiple, dense lines, and makes the code much more
difficult to
follow. And makes refactoring the specific types of (e.g.,) smart pointer
arrays more complicated, and somewhat more dangerous.
Also, I'm quite looking forward to the time when I can write:
GetString(char*, nsContentUtils::PropertiesFile aBundle =
auto::eBRAND_PROPERTIES)
rather than:
GetString(char*, nsContentUtils::PropertiesFile aBundle =
nsContentUtils::PropertiesFile::eBRAND_PROPERTIES)
- Is ranged-for more prone to iterator invalidation errors than the older
form? I believe I've seen .Length() calls hoisted out of old-form loop
conditions pretty frequently. The advantage of ranged-for is claimed to be
that it depends on the operand's iteration API, instead of requiring the
programmer to invent an iteration technique each time they write a loop.
Again, my feeling here is that the opposite is true. If we have a single, de
facto way of writing for loops, it makes it much easier to make sure we
have the correct behavior everywhere. The alternative is separate, ad-hoc
implementations everywhere we iterate over a collection, many of which will
make their own sets of mistakes.
- Are closures more prone to ownership mistakes than the pre-closure
technique? How does this compare with their benefits to legibility?
I think this is the clearest case where the benefits far outweigh the risks.
There are definitely easy-to-overlook lambda capture footguns, but our
static analysis tools are good at preventing those. But there are also huge
benefits.
The ScopeExit class is the best example I can think of. Before we had that
helper, in the best cases, we wound up writing tons of special-purpose RAII
helpers that were hard to think about and maintain. In the worst cases, we
wound up with tons of code that either did not handle early return
correctly at all, or added fragile, duplicated cleanup code in every failure
check `if`. ScopeExit makes our code much safer and more maintainable.
And in the more general case, our pre-lambda code tends to wind up with logic
and data ownership spread across multiple methods and special-purpose
classes (e.g., Runnables), that often get separated in the source, and become
difficult to follow and reason about. Post-lambda, we have abstractions
like MozPromise that make async code much easier to follow, and the data it
owns much more obvious.
For example, this code that I wrote fairly recently:
RefPtr<StreamFilterParent> self(this);
RunOnMainThread(FUNC, [=] {
self->mChannel->Resume();
RunOnActorThread(FUNC, [=] {
if (self->IPCActive()) {
self->CheckResult(self->SendResumed());
}
});
});
Pre-lambda, in the best case would have expanded to something like:
IPCResult
StreamFilterParent::RecvResume()
{
RunOnMainThread(
NewRunnableMethod("StreamFilterParent::Resume1",
this,
StreamFilterParent::Resume1));
return IPC_OK();
}
void
StreamFilterParent::Resume1()
{
mChannel->Resume();
RunOnActorThread(
NewRunnableMethod("StreamFilterParent::Resume2",
this,
StreamFilterParent::Resume2));
}
void
StreamFilterParent::Resume2()
{
if (IPCActive()) {
CheckResult(SendResumed());
}
}
Which is much more difficult to follow (which function runs on which thread?
what data is kept alive across the entire event chain? where do we check
for errors?) and maintain (what happens when I want to add a new event in the
middle of the chain? do I renumber everything and add a new method
declaration to the header?). And that's for a fairly simple case where the only
object held alive is `this`.
When evaluating the impact of new features, we should not let the
familiarity of the mistakes we've been making in C++98 for twenty years
cause us to focus only on the risks from change. That misjudgment would
hurt the quality of the code.
+1
On Mon, Oct 30, 2017 at 8:03 AM, smaug <sm...@welho.com> wrote:
On 10/30/2017 04:52 PM, Simon Sapin wrote:
On 30/10/17 15:05, smaug wrote:
And let's be careful with the new C++ features, pretty please. We
managed to not be careful when we started to use auto, or ranged-for
or lambdas. I'd prefer to not fix more security critical bugs or
memory leaks just because of fancy hip and cool language features ;)
Careful how? How do new language features lead to security bugs? Is new
compiler code not as well tested and could have miscompiles? Are specific
features easy to misuse?
With auto we've managed to hide the ownership of some objects from
reader/reviewer (and I guess also from the patch author),
and this has lead to both to security issues and memory leaks.
Ranged-for lead to security critical crashes when we converted some old
style
for (i = 0; i < array.Length(); ++i) to use it, since ranged-for doesn't
play well when the array changes underneath you.
These days we crash safely there.
With lambdas understanding who owns what becomes harder, and before some
checks, we had (I think rather short while) issues when
there was a raw pointer to a refcounted object captured in a lambda and
the lambda was then dispatched to the event loop.
Nothing guaranteed the captured object to stay alive.
Basically, some "new" features have hidden important aspects of the
lifetime management of objects, and by
doing that, made it easier to write broken code and harder by the
reviewers to catch the mistakes.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
