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.
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.
--
Kris Maglione
Senior Firefox Add-ons Engineer
Mozilla Corporation
APL is a mistake, carried through to perfection. It is the language
of the future for the programming techniques of the past: it creates a
new generation of coding bums.
--Edsger W. Dijkstra
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
