const auto& foo = getFoo();
foo->bar(); // foo is always safe here
Use of auto instead of auto& should be less common. I only use it for:
Casts: `const auto foo = static_cast<uint32_t>(bar);`
Or long (but obvious) types I need a value of, usually RefPtrs of long types.
Almost every other auto is `const auto&`, which has quite good and
predictable behavior: It names a temporary and extends its lifetime to
the current scope.
As for ranged-for:
For one, range-based iteration is equivalent to normal iterator
iteration, which is required for traversal of non-linear containers.
For two, modifying a container while doing normal index iteration
still causes buggy behavior, so it can't be a panacea. (particularly
if you hoist the size!)
for (auto foo : myFoos) {
foo->bar();
}
This is always safe if decltype(foo) is a strong ref. If foo->bar()
can mutate the lifetime of foo, of course you must take a strong
reference to foo. Nothing about auto or range-for changes this. If
foo->bar() can mutate myFoos, even index iteration is likely to have
bugs.
If you don't understand your lifetimes, you get bugs. Keeping to C++03
doesn't save you. Keeping to c++03 just means you only get the c++03
versions of these bugs. (or in our case, ns/moz-prefixed versions of
these bugs)
If you're reviewing code and can't figure out what the lifetimes are:
R-. I know well that sometimes this is the hardest part of review, but
it's a required and important part of the review, and sometimes the
most important precisely because we can't always use a
sufficiently-restricted set of constructs.
Review is sign-off about "I understand this and it's good (enough)".
If you have particular module-specific issues where
otherwise-acceptable constructs are particularly problematic, sure,
review appropriately. In my modules, these constructs are fine.
Perhaps this is because we have a more-constrained problem space than
say dom/html.
TL;DR:
If you have reason to ban a construct for your module: Cool and normal.
But the bar for banning a construct for all modules must be higher than that.
A mismatch for one module is not sufficient reason to ban it in general.
PS: If any reviewers need a crash course in any of these new concepts,
I'm more than happy to set up office hours at the All-Hands.
On Tue, Nov 28, 2017 at 1:35 PM, smaug <[email protected]> wrote:
> On 11/28/2017 06:33 AM, Boris Zbarsky wrote:
>>
>> On 11/27/17 7:45 PM, Eric Rescorla wrote:
>>>
>>> As for the lifetime question, can you elaborate on the scenario you are
>>> concerned about.
>>
>>
>> Olli may have a different concern, but I'm thinking something like this:
>>
>> for (auto foo : myFoos) {
>> foo->bar();
>> }
>>
>
> That was pretty much what I had in mind.
> Though, using auto without range-for, so just
> auto foo = getFoo();
> foo->bar(); // is this safe?
>
>
>
>
>> where bar() can run arbitrary script. Is "foo" held alive across that
>> call? Who knows; you have to go read the definition of the iterators on the
>> type of myFoos to find out.
>>
>> One possible answer is that the right solution for this type of issue is
>> the MOZ_CAN_RUN_SCRIPT static analysis annotation on bar(), which will make
>> this code not compile if the type of "foo" is a raw pointer.... But this
>> annotation is only added to a few functions in our codebase so far, and
>> we'll
>> see how well we manage at adding it to more. We have a _lot_ of stuff in
>> our codebase that can run random script. :(
>>
>> -Boris
>
>
> _______________________________________________
> dev-platform mailing list
> [email protected]
> https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
[email protected]
https://lists.mozilla.org/listinfo/dev-platform
