Re: Code Review bot new reporting style

[Gerald Squelart]

On Friday, March 27, 2020 at 11:11:46 PM UTC+11, Bastien Abadie wrote:
> The code review bot has been updated today and now publishes all issues
> found on your patches as Phabricator lint results (Diff Detail section)
> instead of inline comments.
> 
> Here is a sample revision showcasing the new style.
>  (on phabricator staging)
> 
> 
> The main difference for developers is that lint results are tied to a
> specific patch; so when you update your revision, the lint results are
> automatically removed and only new ones are displayed (if any).
> 
> Please be aware that any Error found by the bot must be fixed before
> landing, or your patch will break the CI. Warnings can still be ignored and
> should not break the build.
> 
> The summary comment remains the same, and will always appear in the
> revision to keep track of the revision evolution.
> 
> Thanks a lot to all the developers
>  who suggested this
> styling change, we hope you’ll enjoy this enhancement.
> 
> If you have any questions regarding this change or the code review bot, you
> can reach us on Matrix #code-review-bot
> .
> 
> 
> Bastien Abadie

It looks nice, thank you! Not seeing expired issues will be great.

> ... Error must be fixed ... Warnings can be ignored ...

Reviewbot used to prefix messages with "Warning" or "Error", but I don't see it 
in Lint messages. How can we make the distinction?

Also Reviewbot comments could be replied to, but I don't see a way to do that 
with Lint messages, is that intended?

Cheers,
Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: How best to do async functions and XPCOM?

[Gerald Squelart]

On Friday, December 6, 2019 at 9:20:21 AM UTC+11, Geoff Lankow wrote:
> Hi all
> 
> I'm redesigning a bunch of Thunderbird things to be asynchronous. I'd 
> like to use Promises but a lot of the time I'll be far from a JS context 
> so that doesn't really seem like an option. The best alternative I've 
> come up with is to create some sort of listener object and pass it to 
> the async function:
> 
> interface nsIFooOperationListener : nsISupports {
>void onOperationComplete(
>  in nsresult status,
>  [optional] in string errorMessage
>);
> };
> 
> ...
> 
> void fooFunction(..., in nsIFooOperationListener listener);
> 
> This works fine but I wonder if there's a better way, or if there's some 
> established prior art I can use/borrow rather than find out the pitfalls 
> myself.
> 
> TIA,
> GL

We have mozilla::MozPromise [0], similar to mozilla::dom::Promise but it 
doesn't rely on JS at all.

It can be a bit tricky to use, the simplest way (to start) is probably to do 
something like InvokeAsync(work thread, code to run that resolves or rejects 
the promise)->Then(target thread, on-success follow-up, on-failure follow-up) 
(e.g., [1]).

Good luck!
g.

[0] 
https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/xpcom/threads/MozPromise.h#98
[1] 
https://searchfox.org/mozilla-central/rev/ea63a0888d406fae720cf24f4727d87569a8cab5/dom/media/ChannelMediaDecoder.cpp#392
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Firefox Profiler now supports recording IPC messages

[Gerald Squelart]

Unfortunately using the pref doesn't always work.

Instead, go to the Firefox hamburger menu -> Web Developer -> Enable Profiler 
Toolbar Icon. It shows up as a small stopwatch.

As Greg said, we're still in transition, sorry for the confusion! ;-)

- Gerald

On Thursday, October 31, 2019 at 8:45:10 AM UTC+11, smaug wrote:
> FWIW, apparently the UI is in the devtools profiler UI, not in the profiler 
> addon.
> https://profiler.firefox.com/ still tells users to install the addon from 
> there.
> 
> I was told that one can get the button similar to the addon by enabling
> devtools.performance.popup.enabled boolean pref and then using 'Customize...'
> 
> Anyhow, great stuff. Seeing IPC messages in the profiles can be really handy.
> (and so far I've been positively surprised that we don't seem to send that 
> many IPC messages)
> 
> 
> -Olli
> 
> 
> 
> On 10/30/19 10:14 PM, Jim Porter wrote:
> > Recently, we landed a new feature for the Firefox Profiler: the ability to 
> > record IPC messages for monitored threads. This should be useful for 
> > evaluating IPC-related performance issues as we make progress on Project 
> > Fission. To enable this feature, just check the "IPC Messages" feature in 
> > the 
> > profiler popup and collect a profile! Then, IPC messages on all monitored 
> > threads will be recorded to the profile.
> > 
> > For an example of what this looks like, see this profile of a user (me) 
> > opening mozilla.org in a new tab: .
> > 
> > Since IPC messages are (obviously) cross-process, each IPC message is 
> > actually comprised of two profiler markers: one for the sending thread and 
> > one 
> > for the receiving thread. The profiler frontend then examines all the 
> > collected IPC markers and correlates the sending and receiving sides. After 
> > correlating each side, we can then determine the latency of the IPC 
> > message: this is defined to be the time between when the message is sent 
> > (i.e. 
> > when `SendMessage` or similar is called) and when it's received (i.e. once 
> > the recipient thread has constructed a `Message` object).
> > 
> > Sometimes, IPC messages will have an unknown duration. This means that the 
> > profiler marker for the other side of the IPC call wasn't recorded (either 
> > the thread wasn't profiled at all or the other side occurred outside of the 
> > time range of the profile).
> > 
> > As you can probably see from the example profile, the user interface is 
> > fairly basic for now: each thread just has a new timeline track to display 
> > its 
> > IPC messages, with outgoing messages in teal and incoming messages in 
> > purple. Of course, there's lots of room for improvement here, so if you 
> > have 
> > ideas for a visualization that would be useful to you, just file a bug and 
> > CC me on it!
> > 
> > Happy profiling!
> > - Jim
> > 
> > P.S.: For those who are curious about how we correlate each side of an IPC 
> > message, we compare the source and destination PIDs, the message's type, 
> > and its seqno. This is enough to uniquely identify any IPC message, though 
> > it does mean that reply messages are considered a separate message. If 
> > people find it useful, it should be straightforward to correlate initial 
> > and reply messages with each other as well.

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: To what extent is sccache's distributed compilation usable?

[Gerald Squelart]

On Tuesday, October 29, 2019 at 3:27:52 AM UTC+11, smaug wrote:
> On 10/24/19 9:17 AM, Marcos Caceres wrote:
> > On Thursday, October 24, 2019 at 6:46:08 AM UTC+11, Christopher Manchester 
> > wrote:
> >> As announced in this week's project call, sccache-dist schedulers have been
> >> deployed to mozilla offices, and those with hardware available can enroll
> >> servers based on the documentation here
> >> .
> > 
> > Just a reminder to not forget about us remotees! We are dying out here with 
> > 1+ hour compile times so anything would really help (even 20-20% 
> > improvement would go a long way). A lot of us have spare old macs, and if 
> > we can put them to good use with this, that would be amazing.
> > 
> 
> 
> Quite often one has just a laptop. Not compiling tons of Rust stuff all the 
> time would be really nice.
> (I haven't figured out when stylo decides to recompile itself - it seems to 
> be somewhat random.)

It's a bit annoying to see things like "force-cargo-...-build" when nothing has 
changed, accompanied by lots of "waiting for file lock on package cache".
It's adding tens of seconds to every build. sccache doesn't help.
I opened https://bugzilla.mozilla.org/show_bug.cgi?id=1568168 for that a while 
ago.

On the sccache topic, if it supports distributed builds, great! I'll have to 
give it a try...
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Passing UniquePtr by value is more expensive than by rref

[Gerald Squelart]

I'm in the middle of watching Chandler Carruth's CppCon talk "There Are No 
Zero-Cost Abstractions" and there's this interesting insight:
https://youtu.be/rHIkrotSwcc?t=1041

The spoiler is already in the title (sorry!), which is that passing 
std::unique_ptr by value is more expensive than passing it by rvalue reference, 
even with no exceptions!

I wrote the same example using our own mozilla::UniquePtr, and got the same 
result: https://godbolt.org/z/-FVMcV (by-value on the left, by-rref on the 
right.)
So I certainly need to recalibrate my gutfeelometer.

A quick searchfox shows a few hundred by-value unique pointer's, we may 
want to look into these.
Though I guess it's a trade-off between the expressiveness of by-value ("I'm 
stealing your value for sure") vs the more efficient but less obvious by-rref 
("Maybe I'll take your value").

And there may be other types we should examine as well?

Cheers,
Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Structured bindings and minimum GCC & clang versions

[Gerald Squelart]

On Friday, August 16, 2019 at 4:19:20 PM UTC+10, Henri Sivonen wrote:
> This week, I wrote some code that made me wish we already had support
> for structured bindings and return by initializer list (both from
> C++17) for mozilla::Tuple.
> 
> That is, if we have
> mozilla::Tuple Foo()
> it would be nice to be able to call it via
> auto [a, b] = Foo();
> and within Foo to write returns as
> return { a, b };
> 
> It appears that our minimum GCC and minimum clang documented at
> https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code
> are pretty old.
> 
> What's the current outlook for increasing the minimum GCC and clang
> versions such that we could start using structured bindings and return
> by initializer list for tuples (either by making sure mozilla::Tuple
> support these or by migrating from mozilla::Tuple to std::tuple) and
> thereby get ergonomic multiple return values in C++?
> 
> -- 
> Henri Sivonen
> hsi...@mozilla.com

I'm guessing that https://bugzilla.mozilla.org/show_bug.cgi?id=1560664 will 
deal with compiler details. (I don't think we can use a C++17 feature without 
enabling C++17 overall -- right?)

I too am eager to start using C++17, mostly constexpr-if.

Cheers,
Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

[Gerald Squelart]

On Wednesday, July 10, 2019 at 9:12:23 AM UTC+10, Bobby Holley wrote:
> On Tue, Jul 9, 2019 at 3:23 PM Mike Hommey  wrote:
> 
> > On Tue, Jul 09, 2019 at 10:39:37AM -0400, Ehsan Akhgari wrote:
> > > On Mon, Jul 8, 2019 at 11:00 PM Gerald Squelart 
> > > wrote:
> > >
> > > > Thank you all for some very interesting discussions so far.
> > > >
> > > > Even if we don't take blanket steps to avoid unsigned types in
> > > > non-bitfield/modulo cases (as suggested by our newly-adopted Google
> > style),
> > > > at least hopefully we're now aware of their subtleties, and we can be
> > more
> > > > careful and deliberate in our choice of integer types in our respective
> > > > domains.
> > > >
> > > > Coming back to my original questions, I think the first part has not
> > been
> > > > categorically answered yet:
> > > >
> > > > Do we have style rules (or folklore) against naked `int`s/`unsigned`s,
> > in
> > > > favor of explicitly-sized `(u)intXX_t` everywhere?
> > > >
> > >
> > > For new code, the style guide for this question can be found here:
> > > https://google.github.io/styleguide/cppguide.html#Integer_Types.  For
> > > existing code, consistency with surrounding code should take precedence
> > for
> > > now.  I hope this answers your question.
> >
> > I thought we only adopted the Google style guide for formatting. Does
> > everything from the guide apply now? Or only parts of it? If the latter,
> > which parts? I'm surprised because I don't remember having seen a mail
> > about this, and surely, I should have noticed something that'd be
> > saying that class member variables names would stop beginning with m,
> > and would instead finish with an underscore and be all lowercase.
> >
> 
> >From the original announcement [1]:
> 
> > We will automatically enforce restrictions on formatting of whitespace
> (such as indentation and braces).
> > For stylistic features other than that (such as naming of functions and
> #include order), Google C++ style
> > will be permitted but not initially enforced, and consistency with
> surrounding code should take precedence.
> 
> In other words, we should default to using Google C++ style when doing so
> would not be massively more disruptive or inconsistent than the
> alternative. So we're not boiling the ocean over mFoo, but preferring the
> explicit integer types and citing the Google style guide is a reasonable
> thing to do.
> 
> [1]
> https://docs.google.com/document/d/1CTaWucldHxEri5BUB1kL4faF4prwPlBa31yHFd-l8uc/edit

That answers my question, thank you Ehsan and Bobby.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

[Gerald Squelart]

Thank you all for some very interesting discussions so far.

Even if we don't take blanket steps to avoid unsigned types in 
non-bitfield/modulo cases (as suggested by our newly-adopted Google style), at 
least hopefully we're now aware of their subtleties, and we can be more careful 
and deliberate in our choice of integer types in our respective domains.

Coming back to my original questions, I think the first part has not been 
categorically answered yet:

Do we have style rules (or folklore) against naked `int`s/`unsigned`s, in favor 
of explicitly-sized `(u)intXX_t` everywhere?

Thanks,
Gerald


On Thursday, July 4, 2019 at 3:11:27 PM UTC+10, Gerald Squelart wrote:
> Recently I coded something with a not-very-important slow-changing 
> rarely-used positive number: `unsigned mGeneration;`
> My reviewer commented: "Please use a type with an explicit size, such as 
> uint32_t. (General mozilla style; you don't see a bare "unsigned" around 
> much)"
> 
> I had never heard of this (in 4+ years), so I did a bit of research:
> 
> - I found plenty of `unsigned`s around, more than `uint32_t`s.
> 
> - I can't see anything about that in our coding style guides [1, 2].
> 
> - Our latest coding style [1] points at Google's, which has a section about 
> Integer Types [3], and the basic gist is: Use plain `int` for "not-too-big" 
> numbers, int64_t for "big" numbers, intXX_t if you need a precise size; never 
> use any unsigned type unless you work with bitfields or need 2^N overflow (in 
> particular, don't use unsigned for always-positive numbers, use signed and 
> assertions instead).
> 
> So, questions:
> 1. Do we have a written style I missed somewhere?
> 2. Do we have an unwritten style? (In which case I think we should write it 
> down.)
> 3. What do we think of the Google style, especially the aversion to unsigned?
> 
> Cheers,
> Gerald
> 
> 
> [1] 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
> [2] https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code
> [3] https://google.github.io/styleguide/cppguide.html#Integer_Types
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

[Gerald Squelart]

(Glad I started this discussion; thank you Nathan for the enlightening links, I 
need to review all my code now!)

Jeff, maybe what we need is a new value type that advertizes that it's 
unsigned, but doesn't have the unwanted 2^N wrapping (and its effects on 
bug-finding tools and compiler optimizations).
`class Unsigned { int mValue; /* magic API here */ }` -- feels like unsigned, 
but underneath it's all `int` arithmetics, with optional >=0 assertions.
Would that help?

Gerald


On Friday, July 5, 2019 at 5:35:30 AM UTC+10, Jeff Gilbert wrote:
> That's what CheckedInt is for, and that's what we use.
> 
> The problems webgl deals with aren't arithmatic. Arithmatic is easy.
> (CheckedInt!) Reasoning about constraints is hard.
> 
> We have some entrypoints where negative values are valid, and many
> where they are not. It's really nice to have a natural way to document
> which we expect /at compile time/. Saying "no unsigned types" really
> throws out the baby with the bathwater for me.
> 
> On Thu, Jul 4, 2019 at 11:46 AM Botond Ballo  wrote:
> >
> > On Thu, Jul 4, 2019 at 2:03 PM Jeff Gilbert  wrote:
> > > It's a huge
> > > help to have a compile-time constraint that values can't be negative.
> >
> > The question is, how useful is that guarantee. Suppose you have some
> > code that decrements an integer too far, past zero. Instead of having
> > a -1 you'll have a 4294967295. Is that an improvement? Will it give
> > the code saner behaviour than the -1?
> >
> > Cheers,
> > Botond

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

[Gerald Squelart]

On Thursday, July 4, 2019 at 4:53:34 PM UTC+10, Boris Zbarsky wrote:
> On 7/4/19 10:11 PM, Gerald Squelart wrote:
> > - I found plenty of `unsigned`s around, more than `uint32_t`s.
> 
> How many are in code that predates the ability to use uint32_t, though?

I didn't do deeper archaeology, so it's very possible most unsigneds are in old 
or 3rd party code. But I didn't want to spend more time on this, in case 
old-timers already had knowledge about it. ;-)

> > - Our latest coding style [1] points at Google's, which has a section about 
> > Integer Types [3], and the basic gist is: Use plain `int` for "not-too-big" 
> > numbers
> 
> If you can 100% guarantee that they will not be too big, right?
> 
> (In particular, for generation counters I would be somewhat worried 
> about making such a guarantee.)

They did add "use int64_t for "big" numbers".

In my own selfish case, it will be once per profiler start/stop. I'm fairly 
confident a user won't start and stop the profiler a few billion times in a 
session. :-D

But that was not the point of this discussion, sorry for the distraction.

> > never use any unsigned type unless you work with bitfields or need 2^N 
> > overflow (in particular, don't use unsigned for always-positive numbers, 
> > use signed and assertions instead).
> 
> Do you happen to know why?  Is this due to worries about underflow or 
> odd behavior on subtraction or something?

Details in OP[3]. Some juicy bits:
"Because of historical accident, the C++ standard also uses unsigned integers 
to represent the size of containers - many members of the standards body 
believe this to be a mistake, but it is effectively impossible to fix at this 
point."
"The fact that unsigned arithmetic doesn't model the behavior of a simple 
integer, but is instead defined by the standard to model modular arithmetic 
(wrapping around on overflow/underflow), means that a significant class of bugs 
cannot be diagnosed by the compiler."
"Mixing signedness of integer types is responsible for an equally large class 
of problems."

> -Boris

g.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Coding style  : `int` vs `intX_t` vs `unsigned/uintX_t`

[Gerald Squelart]

Recently I coded something with a not-very-important slow-changing rarely-used 
positive number: `unsigned mGeneration;`
My reviewer commented: "Please use a type with an explicit size, such as 
uint32_t. (General mozilla style; you don't see a bare "unsigned" around much)"

I had never heard of this (in 4+ years), so I did a bit of research:

- I found plenty of `unsigned`s around, more than `uint32_t`s.

- I can't see anything about that in our coding style guides [1, 2].

- Our latest coding style [1] points at Google's, which has a section about 
Integer Types [3], and the basic gist is: Use plain `int` for "not-too-big" 
numbers, int64_t for "big" numbers, intXX_t if you need a precise size; never 
use any unsigned type unless you work with bitfields or need 2^N overflow (in 
particular, don't use unsigned for always-positive numbers, use signed and 
assertions instead).

So, questions:
1. Do we have a written style I missed somewhere?
2. Do we have an unwritten style? (In which case I think we should write it 
down.)
3. What do we think of the Google style, especially the aversion to unsigned?

Cheers,
Gerald


[1] 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
[2] https://developer.mozilla.org/en-US/docs/Mozilla/Using_CXX_in_Mozilla_code
[3] https://google.github.io/styleguide/cppguide.html#Integer_Types
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Rust required to build Gecko

[Gerald Squelart]

On Tuesday, November 29, 2016 at 5:03:17 AM UTC+11, Gregory Szorc wrote:
> > On Nov 27, 2016, at 14:46, Gerald Squelart <sque...@gmail.com> wrote:
> >
> >> On Saturday, November 26, 2016 at 3:59:05 AM UTC+11, Ralph Giles wrote:
> >> On Fri, Nov 25, 2016 at 7:48 AM, Andrew Halberstadt
> >> <ahalbe...@mozilla.com> wrote:
> >>
> >>> For anyone confused by this, the binaries are downloaded to ~/.cargo/bin
> >>> and adding this directory to your $PATH should fix the issue. The
> >>> bootstrapper explains this if you run it a second time, but makes no
> >>> mention of it the first time through for some reason.
> >>
> >> Thanks. I've tried to address this in
> >> https://bugzilla.mozilla.org/show_bug.cgi?id=1319860
> >>
> >> -r
> >
> > Hi Ralph,
> >
> > Following your instructions, rustc and friends are in ~/.cargo/bin, and 
> > I've added that path in my $PATH.
> >
> > But now, `./mach build` gives me:
> >  force-cargo-build
> >  env: /usr/local/bin/cargo: No such file or directory
> >  /mozilla-central/config/rules.mk:951: recipe for target 
> > 'force-cargo-build' failed
> >  gmake[2]: *** [force-cargo-build] Error 127
> >  /mozilla-central/config/recurse.mk:71: recipe for target 
> > 'toolkit/library/rust/target' failed
> >  gmake[1]: *** [toolkit/library/rust/target] Error 2
> >  gmake[1]: *** Waiting for unfinished jobs
> >
> > Sym-linking rustc from /usr/local/bin worked, but it feels wrong!
> 
> This is a longstanding problem with the build system w.r.t. detected
> programs changing out from under the build system: the build system
> makes a lot of assumptions that the system doesn't change between
> builds. If you do things like change the toolchain or where binaries
> are installed, you'll need to perform a clobber build.

Ah yes, it worked after a clobber. Sorry, I should have tried that before.

> I just filed bug 1320728 to improve the build system's handling of
> this scenario.

Thank you!
g.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Rust required to build Gecko

[Gerald Squelart]

On Saturday, November 26, 2016 at 3:59:05 AM UTC+11, Ralph Giles wrote:
> On Fri, Nov 25, 2016 at 7:48 AM, Andrew Halberstadt
>  wrote:
> 
> > For anyone confused by this, the binaries are downloaded to ~/.cargo/bin
> > and adding this directory to your $PATH should fix the issue. The
> > bootstrapper explains this if you run it a second time, but makes no
> > mention of it the first time through for some reason.
> 
> Thanks. I've tried to address this in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1319860
> 
>  -r

Hi Ralph,

Following your instructions, rustc and friends are in ~/.cargo/bin, and I've 
added that path in my $PATH.

But now, `./mach build` gives me:
  force-cargo-build
  env: /usr/local/bin/cargo: No such file or directory
  /mozilla-central/config/rules.mk:951: recipe for target 'force-cargo-build' 
failed
  gmake[2]: *** [force-cargo-build] Error 127
  /mozilla-central/config/recurse.mk:71: recipe for target 
'toolkit/library/rust/target' failed
  gmake[1]: *** [toolkit/library/rust/target] Error 2
  gmake[1]: *** Waiting for unfinished jobs

Sym-linking rustc from /usr/local/bin worked, but it feels wrong!


After that, building works, but shows some scary bold text:
  note: link against the following native artifacts when linking against this 
static library
  note: the order and any duplication can be significant on some platforms, and 
so may need to be preserved
  note: library: System
  note: library: c
  note: library: m

What's with that?

Cheers,
g.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Linux content sandbox tightened

[Gerald Squelart]

On Friday, October 7, 2016 at 6:49:53 PM UTC+11, Gian-Carlo Pascutto wrote:
> Hi all,
> 
> the next Nightly build will have a significantly tightened Linux
> sandbox. Writes are no longer allowed except to shared memory (for IPC),
> and to the system TMPDIR (and we're eventually going to get rid of the
> latter, perhaps with an intermediate step to a Firefox-content-specific
> tmpdir).
> 
> There might be some compatibility fallout from this. Extensions/add-ons
> that try to write from the content process will no longer work, but the
> impact there should be limited given that similar (and stricter)
> restrictions have been tried out on macOS. (See bug 1187099 and bug
> 1288874 for info/discussion). Because Firefox currently still loads a
> number of external libraries into the content process (glib, gtk,
> pulseaudio, etc) there is some risk of breakage there as well. You know
> where to report (Component: Security - Process Sandboxing).
> 
> This behavior can be controlled via a pref:
> pref("security.sandbox.content.level", 2);
> 
> Reverting this to 1 goes back to the previous behavior where the set of
> allowable system calls is restricted, but no filtering happens on
> filesytem IO.
> 
> When Firefox is built with debugging enabled, it will log any policy
> violations. Currently, a clean Nightly build will show some of those.
> They are inconsequential, and we'll deal with them, eventually. (Patches
> welcome though!)
> 
> -- 
> GCP

Hi Gian-Carlo,

It seems this tightening is now preventing us from using ALSA:
https://bugzilla.mozilla.org/show_bug.cgi?id=1247056#c167

Coincidentally, we have just disabled ALSA by default, but the code is still 
there and can be enable in builds, so it'd be nice to allow its use for 
power-users who are still stuck with it, if that's possible.
I'll probably open a new bug about that soon. Is there some meta-bug we can 
block? (I couldn't find any.) Or just NI you?

Cheers,
Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: NS_WARN_IF_FALSE has been renamed NS_WARNING_ASSERTION

[Gerald Squelart]

On Thursday, September 22, 2016 at 5:58:58 PM UTC+10, Paul Adenot wrote:
> On Thu, Sep 22, 2016, at 07:33 AM, Gerald Squelart wrote:
> > Sitting on the shoulders of giants, an idea, in the unlikely case it's
> > not been thought of yet:
> > How about an assertion that files a crash report (or something lighter
> > like a telemetry blip) but does not actually crash?
> 
> I think you can do that with telemetry, we do that, for example, when we
> encounter a severe failure when trying to open audio devices [0], trying
> to get a sense of how frequent it is in the field.
> 
> Cheer,
> Paul.
> 
> [0]:
> http://searchfox.org/mozilla-central/source/dom/media/GraphDriver.cpp#595

Nice one Paul.

But I was hoping for something easier and more flexible than telemetry, where I 
can just write an assertion, and something will land in Socorro when it fails, 
but without crashing Firefox.

Cheers,
Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: NS_WARN_IF_FALSE has been renamed NS_WARNING_ASSERTION

[Gerald Squelart]

On Thursday, September 22, 2016 at 2:58:46 PM UTC+10, Daniel Holbert wrote:
> On 09/21/2016 08:41 PM, Samael Wang wrote:
> > The NS_ASSERTION document [1] says "Don't use NS_ASSERTION", which could be 
> > a bit confusing that now some of the similar named macros may be deprecated 
> > but some are new and encouraged.
> 
> I think that document's advice is too severe.
> 
> roc made a compelling case for *preferring* non-fatal assertions (i.e.
> NS_ASSERTION) in many scenarios, particularly when failure is not
> catastrophic.  See his blog post here for more details:
>  http://robert.ocallahan.org/2011/12/case-for-non-fatal-assertions.html
> 
> I found (and still find) his argument there quite compelling.  I think
> it's important that debug builds are able to proceed past (inevitable)
> unexpected-but-not-horrible conditions -- while at the same time,
> perhaps shouting about those unexpected conditions noisily so that
> people can report them if they care to do so (and so that test suites fail).
> 
> Personally, I use *both* MOZ_ASSERT and NS_ASSERTION, in different
> scenarios, depending on how catastrophic it would be if the asserted
> condition doesn't hold, and depending on how sure I am that the
> condition does-currently & must-always-in-the-future hold true.
> [...]
> ~Daniel

Sitting on the shoulders of giants, an idea, in the unlikely case it's not been 
thought of yet:
How about an assertion that files a crash report (or something lighter like a 
telemetry blip) but does not actually crash?

g.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New [must_use] property in XPIDL

[Gerald Squelart]

On the build-time vs static analysis point:

I'd much prefer to have errors pointed out right from './mach build', which I 
can fix on the spot; rather than wait hours until I notice static analysis 
errors on a treeherder build.
(e.g., I always forget explicit/MOZ_IMPLICIT!)

Unless we can get cross-platform & consistent static analysis from our build 
environments?

g.

On Tuesday, August 23, 2016 at 11:20:30 PM UTC+10, Eric Rescorla wrote:
> I'm indifferent to this particular case, but I think that rkent's point
> about static
> checking is a good one. Given that C++ has existing annotations that say:
> 
> - This does not produce a useful return value (return void)
> - I am explicitly ignoring the return value (cast to void)
> 
> And that we have (albeit imperfect) static checking tools that can detect
> non-use of
> return values, it seems like we would ultimately be better-off by using
> those tools
> to treat use of the return value by the default flagging a compiler error
> when that
> doesn't happen yet a third annotation rather than treating "use the return
> value
> somehow" as the default and flagging a compiler error when that doesn't
> happen.
> I appreciate that we have a lot of code that violates this rule now, so
> actually cleaning
> that up is a long process gradually converting the code base, but it has
> the advantage
> that once that's done then it just stays clean (just like any other -Werror
> conversion).
> 
> -Ekr
> 
> 
> On Mon, Aug 22, 2016 at 5:03 PM, Bobby Holley  wrote:
> 
> > On Mon, Aug 22, 2016 at 4:39 PM, R Kent James  wrote:
> >
> > > On 8/21/2016 9:14 PM, Nicholas Nethercote wrote:
> > > > I strongly encourage people to do likewise on
> > > > any IDL files with which they are familiar. Adding appropriate checks
> > > isn't
> > > > always easy
> > >
> > > Exactly, and I hope that you and others restrain your exuberance a
> > > little bit for this reason. A warning would be one thing, but a hard
> > > failure that forces developers to drop what they are doing and think
> > > hard about an appropriate check is just having you set YOUR priorities
> > > for people rather than letting people do what might be much more
> > > important work.
> > >
> > > There's lots of legacy code around that may or may not be worth the
> > > effort to think hard about such failures. This is really better suited
> > > for a static checker that can make a list of problems, which list can be
> > > managed appropriately for priority, rather than a hard error that forces
> > > us to drop everything.
> > >
> >
> > I don't quite follow the objection here.
> >
> > Anybody who adds such an annotation needs to get the tree green before they
> > land the annotation. Developers writing new code that ignores the nsresult
> > will get instant feedback (by way of try failure) that the developer of the
> > API thinks the nsresult needs to be checked. This doesn't seem like an
> > undue burden, and enforced-by-default assertions are critical to code
> > hygiene and quality.
> >
> > If your concern is the way this API change may break Thunderbird-only
> > consumers of shared XPCOM APIs, that's related to Thunderbird being a
> > non-Tier-1 platform, and pretty orthogonal to the specific change that Nick
> > made.
> >
> > bholley
> >
> > > :rkent
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Welcome new Data Stewardship Peers

[Gerald Squelart]

On Thursday, July 14, 2016 at 6:51:21 AM UTC+10, Benjamin Smedberg wrote:
> I'd like to welcome Chenxia Liu, François Marier, and Rebecca Weiss as
> peers of Firefox Data Stewardship, joining Ally and myself. I'm excited to
> bring a selection of people from across the organization who are familiar
> with different products and projects within Mozilla, and I hope that this
> reduces the time needed for people to get review.
> 
> As a reminder, all data collection from the Firefox products, including
> changes to telemetry histograms, should be reviewed by a data steward peer
> before landing. More information about Firefox data collection can be found
> at https://wiki.mozilla.org/Firefox/Data_Collection
> 
> Please bear with the new peers as they learn the routine: it may be a few
> days or weeks until they are fully up to speed.
> 
> --BDS

Welcome to the new blood!


While I've got you here, can we please discuss one small details of the review 
process?

The wiki page reads "please request approval by setting the *feedback* flag for 
the data collection module owner or a peer".
With mozreview becoming more prevalent (and easy to use) for reviews, could we 
please just use the *review* flag instead? (At least for now*)
Of course the data collection reviewer should only look at the data collection 
side, another reviewer should be nominated to look at the code.

* A little bird told me that eventually the feedback flag will become a 
first-class citizen in mozreview. ;-)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: MXR permanently offline, please transition to DXR

[Gerald Squelart]

On Saturday, July 9, 2016 at 12:57:31 PM UTC+10, Boris Zbarsky wrote:
> On 7/8/16 7:25 PM, Gerald Squelart wrote:
> > The annotate (aka blame) functionality of hg.mozilla.org can point at one 
> > line
> 
> Yes, I know.  What it can't do is highlight some set of lines containing 
> more than one line.  Think things like:
> 
>http://mxr.mozilla.org/whatever-file?mark=10-20,17,25#8
> 
> which highlights lines 10-20, 17, and 25, and scrolls to line 8.  This 
> is very useful when pointing people at code.
> 
> -Boris

Ooh, didn't know about that one, nifty!
It would be great to get it into hgweb (as you mention in your reply to Eric).

g.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: MXR permanently offline, please transition to DXR

[Gerald Squelart]

On Saturday, July 9, 2016 at 7:21:24 AM UTC+10, Boris Zbarsky wrote:
> On 7/8/16 4:18 PM, Steve Fink wrote:
> > Are either mxr or dxr really the right thing for canonical links to our
> > source code? As long as we're updating links, through one means or
> > another, temporarily or permanently, couldn't we come up with a set of
> > urls that would be better in the long term? One that clearly states the
> > purpose of a link (eg a link to the latest version of a whole file vs a
> > link to a range of lines in a specific version vs a link to a named
> > function definition or whatever), and could be redirected when the next
> > great thing comes along?
> 
> Yes, please!
> 
> Especially because DXR doesn't allow independent specification of "lines 
> to highlight" and "line to scroll to", so any conversion from mxr links 
> to dxr ones will be somewhat lossy...
> 
> There's still the question of what to back the links with of course 
> (that is, what runs on the server).
> 
> > hg.mozilla.org would seem to be better for versioned links
> 
> Can't highlight lines there, last I checked.
> 
> -Boris

The annotate (aka blame) functionality of hg.mozilla.org can point at one line, 
e.g.:
https://hg.mozilla.org/mozilla-central/annotate/c2da34d96746288b5fee27bf6542a12c9f410988/dom/media/platforms/PDMFactory.cpp#l126
(Hash, lowercase 'L', line number)
I've opened https://bugzil.la/1282329 requesting that DXR generates these as 
recommended permalinks -- Hope you like it. :-)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Monday, May 2, 2016 at 9:49:24 AM UTC+10, Jim Blandy wrote:
> No, we don't know that [moved-from strings become empty].
> The contract of a move in C++ is simply that the
> source object is safe to destruct, but otherwise in an undefined state*. You
> must not make any assumptions about its value.
* (To be fair, in that context I think you were only arguing there that we 
can't assume anything about the value, but didn't explicitly rule out a 
reassignment.)

Sorry to revive this old thread, but I've just noticed that our MFBT Swap() is 
written in terms of Move()'s:
> template
> inline void
> Swap(T& aX, T& aY)
> {
>   T tmp(Move(aX));
>   aX = Move(aY);
>   aY = Move(tmp);
> }
This assumes that a moved-from object can safely be assigned to -- Which used 
to seem reasonable to me, but I got confused after our discussions here!

Clang's std::swap does the same! (With extra checks that the type is move 
constructible)

So can we assume that any moved-from object should be left in a state that is 
safe to destruct AND safe to be assigned to?

But no assumption can be made about the actual contained value, i.e.: It could 
be the same, it could be empty, or it could be in some unsafe-to-use state 
until destroyed or overwritten.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Return-value-optimization when return type is RefPtr

[Gerald Squelart]

On Friday, June 17, 2016 at 3:57:01 PM UTC+1, Gerald Squelart wrote:
> On Friday, June 17, 2016 at 2:31:15 PM UTC+1, smaug wrote:
> > On 06/16/2016 06:40 PM, smaug wrote:
> > > On 05/24/2016 08:33 AM, jw...@mozilla.com wrote:
> > >> For
> > >>
> > >> RefPtr GetFoo() {
> > >>RefPtr foo;
> > >>// ...
> > >> }
> > >>
> > >> should we:
> > >>
> > >> 1. return foo and expect RVO to kick in to eliminate additional 
> > >> AddRef/Release
> > >> 2. return foo.forget()
> > >> 3. return Move(foo)
> > >>
> > >> Which one is preferred?
> > >>
> > >> ps: I find gcc is able to apply RVO even with "-O0". Not sure if it is 
> > >> also true for other compilers.
> > >>
> > >
> > > Can we somehow guarantee that RVO kicks in? It feels quite error prone to 
> > > rely on compiler optimization when dealing with
> > > possibly slow Addref/Release calls.
> > > https://en.wikipedia.org/wiki/Return_value_optimization#Compiler_support 
> > > hints that there are cases when RVO might not happen.
> > >
> > 
> > I've been told we can't guarantee RVO to kick in, which means, I think, 
> > that we should assume we don't have RVO ever and need to
> > rely on other coding patterns which are guaranteed to optimize out extra 
> > addref/release.
> > Currently having already_AddRefed as the return type is one such pattern.
> > 
> > What are the other options?
> > 
> > 
> > At least we can't start using RefPtr as return type before we know it 
> > won't be causing performance regressions.
> > So I would recommend using already_AddRefed for now.
> 
> From what *I* understand, RVO is guaranteed (or at least supported 
> everywhere?) when there is only one stack variable that is returned, e.g.:
> C foo()
> {
>   C rv;
>   // ... (put stuff in rv)
>   return rv;
> }
> In this case, the caller function stack can host 'rv' directly, no copies 
> needed.
> 
> RVO won't happen when there may be multiple variables in the function, only 
> one of which will be returned, e.g.:
> C foo()
> {
>   C rv1, rv2; bool b;
>   // ... (assign a value to b)
>   return b ? rv1 : rv2;
> }
> The caller cannot know which of rv1 or rv2 will be returned, so no return 
> value can be lifted.
> 
> (Any actual expert who knows the truth?)
(sorry for the spam)

If I'm correct, then the question becomes: Can we trust developers to always 
make the right choice? Or better: Can we write static analyzers that will catch 
RVO-hostile code?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Return-value-optimization when return type is RefPtr

[Gerald Squelart]

On Friday, June 17, 2016 at 2:31:15 PM UTC+1, smaug wrote:
> On 06/16/2016 06:40 PM, smaug wrote:
> > On 05/24/2016 08:33 AM, jw...@mozilla.com wrote:
> >> For
> >>
> >> RefPtr GetFoo() {
> >>RefPtr foo;
> >>// ...
> >> }
> >>
> >> should we:
> >>
> >> 1. return foo and expect RVO to kick in to eliminate additional 
> >> AddRef/Release
> >> 2. return foo.forget()
> >> 3. return Move(foo)
> >>
> >> Which one is preferred?
> >>
> >> ps: I find gcc is able to apply RVO even with "-O0". Not sure if it is 
> >> also true for other compilers.
> >>
> >
> > Can we somehow guarantee that RVO kicks in? It feels quite error prone to 
> > rely on compiler optimization when dealing with
> > possibly slow Addref/Release calls.
> > https://en.wikipedia.org/wiki/Return_value_optimization#Compiler_support 
> > hints that there are cases when RVO might not happen.
> >
> 
> I've been told we can't guarantee RVO to kick in, which means, I think, that 
> we should assume we don't have RVO ever and need to
> rely on other coding patterns which are guaranteed to optimize out extra 
> addref/release.
> Currently having already_AddRefed as the return type is one such pattern.
> 
> What are the other options?
> 
> 
> At least we can't start using RefPtr as return type before we know it 
> won't be causing performance regressions.
> So I would recommend using already_AddRefed for now.

>From what *I* understand, RVO is guaranteed (or at least supported 
>everywhere?) when there is only one stack variable that is returned, e.g.:
C foo()
{
  C rv;
  // ... (put stuff in rv)
  return rv;
}
In this case, the caller function stack can host 'rv' directly, no copies 
needed.

RVO won't happen when there may be multiple variables in the function, only one 
of which will be returned, e.g.:
C foo()
{
  C rv1, rv2; bool b;
  // ... (assign a value to b)
  return b ? rv1 : rv2;
}
The caller cannot know which of rv1 or rv2 will be returned, so no return value 
can be lifted.

(Any actual expert who knows the truth?)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Return-value-optimization when return type is RefPtr

[Gerald Squelart]

Coincidentally: (?)
https://bugzilla.mozilla.org/show_bug.cgi?id=1280296
"remove already_AddRefed"
:-)


For your original question, I would vote for RVO when possible, and Move() 
otherwise.

It feels like static analysis should be able to detect cases where RVO is 
possible and suggest it if missing; and also detect cases where it's not 
possible and warn about a missing Move().
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Monday, May 2, 2016 at 9:49:24 AM UTC+10, Jim Blandy wrote:
> On Fri, Apr 29, 2016 at 4:43 PM, Gerald Squelart <squel...@gmail.com> wrote:
> 
> > For example, we know how strings behave when moved from* (the original
> > becomes empty), and it'd be nice to be able to use that trick when possible
> > and really needed.
> >
> 
> No, we don't know that. The contract of a move in C++ is simply that the
> source object is safe to destruct, but otherwise in an undefined state. You
> must not make any assumptions about its value.

"contract of a move" -- Where is this written? (Really: If there is an official 
position on this, I'd like to see it!)

The little bit of legalese I could find was about the std:
"""
17.6.5.15 Moved-from state of library types [lib.types.movedfrom]
Objects of types defined in the C++ standard library may be moved from. Move 
operations may be explicitly specified or implicitly generated. Unless 
otherwise specified, such moved-from objects shall be placed in a valid but 
unspecified state.
"""

"Valid but unspecified" is quite different from the loaded word "undefined".
"Valid" to me means that operations should still be possible on the object, but 
the results will just depends on what's left in there after the move.

And one specific example:
"""
30.3.1 Class thread [thread.thread.class]
[...] Objects of class thread can be in a state that does not represent a 
thread of execution. [ Note: A thread object does not represent a thread of 
execution after default construction, after being moved from, or after a 
successful call to detach or join. -- end note ]
"""
This to me implies that a 'thead' object could be moved from, and after that 
its state is valid but does not actually represent a thread of execution.

> It is not always the case that the fastest move implementation leaves the
> source empty. For example, if the string is using inline storage, then a
> move would need to take extra steps to clear the original.

Agreed, sometimes copy is faster, e.g., for PODs, or yes, objects with inline 
storage.

> You write about "us[ing] that trick when possible and really needed", when
> what you're actually saying is "let's depend on undefined behavior." That
> approach is common, and its history is not pretty.

My (more-restricted) point from the following post is that *we* can decide what 
some objects will contain after a move, and we could work with that *defined* 
behavior where possible...

And because it could be misused as you fear, we could introduce annotations to 
ensure that that kind of use is controlled. E.g. Mark reusable-after-move 
objects as such, and use a different word than 'Move' when making these objects 
xvalues. And use type traits so optionally take this optimized path from 
generic functions.


Thinking of it, I suppose lots (all?) of these optimized content-stealing 
actions could be done through differently-named methods (e.g. 'Take()'), so 
they could not possibly be confused with C++ move semantics.
So I could live with a communal decision to forbid any use-after-move ever.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Saturday, April 30, 2016 at 9:43:46 AM UTC+10, Gerald Squelart wrote:
> On Friday, April 29, 2016 at 2:15:03 PM UTC+10, Jim Blandy wrote:
> > On Thu, Apr 28, 2016 at 8:22 PM, <jw...@mozilla.com> wrote:
> > 
> > > Jim Blandy於 2016年4月28日星期四 UTC+8下午1時51分15秒寫道：
> > > > I don't really think it's a good example. TakeMediaIfKnown is accepting 
> > > > a
> > > > UniquePtr as an inout parameter: it uses, and may modify, its
> > > > value. It should take UniquePtr &.
> > > IIUC, UniquePtr can't be an inout parameter for its unique semantics
> > > which owns sole ownership of an object. The caller won't be able to use 
> > > the
> > > object in a meaningful way after the function returns.
> > >
> > >
> > >
> > I'm not sure I understand. Maybe it would help if we had a more concrete
> > example to talk about:
> > 
> > $ cat unique-inout.cc
> > #include 
> > #include "mozilla/UniquePtr.h"
> > 
> > using mozilla::UniquePtr;
> > 
> > struct MediaFile {
> >   const char *name;
> >   MediaFile(const char *name) : name(name) { printf("constructing %s\n",
> > name); }
> >   ~MediaFile() { printf("destructing %s\n", name); }
> > };
> > 
> > int foo(UniquePtr , bool pleaseSwap)
> > {
> >   if (pleaseSwap) {
> > UniquePtr ptr = Move(arg);
> > arg = UniquePtr(new MediaFile("foo's"));
> >   }
> > }
> > 
> > int main(int argc, char **argv) {
> >   UniquePtr first(new MediaFile("first"));
> >   printf("before first call\n");
> >   foo(first, false);
> >   printf("after first call\n");
> > 
> >   UniquePtr second(new MediaFile("second"));
> >   printf("before second call\n");
> >   foo(second, true);
> >   printf("after second call\n");
> > 
> > }
> > $ ln -sf /home/jimb/moz/dbg/mfbt mozilla
> > $ g++ -std=c++11 -I . unique-inout.cc -o unique-inout
> > $ ./unique-inout
> > constructing first
> > before first call
> > after first call
> > constructing second
> > before second call
> > constructing foo's
> > destructing second
> > after second call
> > destructing foo's
> > destructing first
> > $
> > 
> > The first MediaFile's destructor doesn't run until the end of main. The
> > second MediaFile's destructor runs during the second call to foo, and then
> > foo's MedialFile's destructor runs at the end of main.
> > 
> > That's what I meant by a function taking a UniquePtr as an inout parameter.
> > It seemed to me like the function Gerald imagined should be written as in
> > my code above, rather than passing Move(...) as the argument.
> 
> I guess my example was not that convincing. :-P
> 
> What about a function that takes T&, but then wants to "steal" the contents 
> of T? (Say, to emplace it into an array.)
> The most efficient way is to Move() it to a 2nd function that takes T&&. But 
> it means that the first-level function will now have a shell of that object 
> that was passed as an lvalue ref, so maybe its caller will still use that 
> object somehow.
> (Once again, just thinking of the possibilities, maybe there's no actual need 
> for that pattern in real life.)
> For example, we know how strings behave when moved from* (the original 
> becomes empty), and it'd be nice to be able to use that trick when possible 
> and really needed.
> 
> If we want to discourage that as well, I guess we'd need to enforce another 
> restriction, e.g. "you cannot Move() an lvalue reference".
> 
> > Although the
> > language doesn't enforce it, I think Move should be reserved for
> > unconditional transfers of ownership.
> 
> And I say: Yes, let's agree that Move() *will* be reserved for unconditional 
> transfers.
> But please allow for an escape hatch, e.g. 'MakeAvailableForMove/MaybeMove'** 
> or similar, just in case.
> 
> 
> * Regarding the validity of moved-from objects:
> I can think of 3 categories:
> 1. Objects that survive being moved-from, and are totally reusable, with the 
> default behavior of just being empty -- whatever that means in each case, 
> e.g. containers become empty, pointers become null, etc.
> 2. Objects that should only be assigned-to through operator=(), e.g. physical 
> resource handlers.
> 3. Objects that shouldn't be touched at all (except destruction).
> (And maybe objects that are semi-functional, e.g. you can use some methods 
> but not others; This seem

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Friday, April 29, 2016 at 2:15:03 PM UTC+10, Jim Blandy wrote:
> On Thu, Apr 28, 2016 at 8:22 PM,  wrote:
> 
> > Jim Blandy於 2016年4月28日星期四 UTC+8下午1時51分15秒寫道：
> > > I don't really think it's a good example. TakeMediaIfKnown is accepting a
> > > UniquePtr as an inout parameter: it uses, and may modify, its
> > > value. It should take UniquePtr &.
> > IIUC, UniquePtr can't be an inout parameter for its unique semantics
> > which owns sole ownership of an object. The caller won't be able to use the
> > object in a meaningful way after the function returns.
> >
> >
> >
> I'm not sure I understand. Maybe it would help if we had a more concrete
> example to talk about:
> 
> $ cat unique-inout.cc
> #include 
> #include "mozilla/UniquePtr.h"
> 
> using mozilla::UniquePtr;
> 
> struct MediaFile {
>   const char *name;
>   MediaFile(const char *name) : name(name) { printf("constructing %s\n",
> name); }
>   ~MediaFile() { printf("destructing %s\n", name); }
> };
> 
> int foo(UniquePtr , bool pleaseSwap)
> {
>   if (pleaseSwap) {
> UniquePtr ptr = Move(arg);
> arg = UniquePtr(new MediaFile("foo's"));
>   }
> }
> 
> int main(int argc, char **argv) {
>   UniquePtr first(new MediaFile("first"));
>   printf("before first call\n");
>   foo(first, false);
>   printf("after first call\n");
> 
>   UniquePtr second(new MediaFile("second"));
>   printf("before second call\n");
>   foo(second, true);
>   printf("after second call\n");
> 
> }
> $ ln -sf /home/jimb/moz/dbg/mfbt mozilla
> $ g++ -std=c++11 -I . unique-inout.cc -o unique-inout
> $ ./unique-inout
> constructing first
> before first call
> after first call
> constructing second
> before second call
> constructing foo's
> destructing second
> after second call
> destructing foo's
> destructing first
> $
> 
> The first MediaFile's destructor doesn't run until the end of main. The
> second MediaFile's destructor runs during the second call to foo, and then
> foo's MedialFile's destructor runs at the end of main.
> 
> That's what I meant by a function taking a UniquePtr as an inout parameter.
> It seemed to me like the function Gerald imagined should be written as in
> my code above, rather than passing Move(...) as the argument.

I guess my example was not that convincing. :-P

What about a function that takes T&, but then wants to "steal" the contents of 
T? (Say, to emplace it into an array.)
The most efficient way is to Move() it to a 2nd function that takes T&&. But it 
means that the first-level function will now have a shell of that object that 
was passed as an lvalue ref, so maybe its caller will still use that object 
somehow.
(Once again, just thinking of the possibilities, maybe there's no actual need 
for that pattern in real life.)
For example, we know how strings behave when moved from* (the original becomes 
empty), and it'd be nice to be able to use that trick when possible and really 
needed.

If we want to discourage that as well, I guess we'd need to enforce another 
restriction, e.g. "you cannot Move() an lvalue reference".

> Although the
> language doesn't enforce it, I think Move should be reserved for
> unconditional transfers of ownership.

And I say: Yes, let's agree that Move() *will* be reserved for unconditional 
transfers.
But please allow for an escape hatch, e.g. 'MakeAvailableForMove/MaybeMove'** 
or similar, just in case.


* Regarding the validity of moved-from objects:
I can think of 3 categories:
1. Objects that survive being moved-from, and are totally reusable, with the 
default behavior of just being empty -- whatever that means in each case, e.g. 
containers become empty, pointers become null, etc.
2. Objects that should only be assigned-to through operator=(), e.g. physical 
resource handlers.
3. Objects that shouldn't be touched at all (except destruction).
(And maybe objects that are semi-functional, e.g. you can use some methods but 
not others; This seems too hard to reason with anyway, so let's not go there.)

It'd be nice to discuss whether we could auto-classify all our types 
into one default group (looks like we're going for '3' here) and allow 
annotations to move some to other groups; Move semantics could then be tailored 
by types.


** Tangent:
It'd be nice to be able to mark some things as 
'MOZ_ARE_YOU_SURE_YOU_WANT_TO_USE_THIS', and ReviewBoard would make them very 
obvious, to ensure extra attention during reviews.
Or require review from a chosen few (similar to dom peers having to review dom 
changes).
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: MOZ_WARN_UNUSED_RESULT has been renamed as MOZ_MUST_USE

[Gerald Squelart]

On Friday, April 29, 2016 at 8:04:46 PM UTC+10, Nicholas Nethercote wrote:
> On Fri, Apr 29, 2016 at 10:25 AM, Nicholas Nethercote
>  wrote:
> >
> > I just landed on inbound the patches in bug 1267550, which renamed
> > MOZ_WARN_UNUSED_RESULT as MOZ_MUST_USE.
> >
> > A shorter name was in order because it's an attribute that is already
> > used widely, and should be used even more, because it catches real
> > problems. In fact, any function that is fallible and returns a
> > non-pointer value (usually a bool or nsresult) is a candidate for a
> > MOZ_MUST_USE annotation.
> 
> I've created bug 1268766 as a meta-bug for tracking the addition of
> more uses of MOZ_MUST_USE. I've started working on this in a few
> places -- and already found a bunch of missing failure checks -- but
> we have more than enough code for the task to be shared around, if
> anybody is interested.
> 
> Also note that we have another attribute called MOZ_MUST_USE_TYPE,
> which can be applied to a type, and then any function that returns
> that type will need to be checked. (In other words, it implicitly adds
> MOZ_MUST_USE to any function that returns that type.) Bug 1209780 is a
> nice example of its use. (Note that it used to be called MOZ_MUST_USE,
> and was renamed MOZ_MUST_USE_TYPE to allow MOZ_WARN_UNUSED_RESULT to
> be renamed as MOZ_MUST_USE.) However, it only works in static analysis
> builds, whereas MOZ_MUST_USE also works in GCC and clang, so although
> it clutters up code less, it also results in less immediate error
> messages.
> 
> Nick

Thank you for that.


Now, for maximum defensiveness, shouldn't we go even further?

How about: Make 'MOZ_MUST_USE' implicit for all functions/methods (except void 
of course, probably methods returning T&, and maybe more as they come up).
When a result is not needed somewhere, use the 'Unused << foo()' idiom.
And if a function's return is really not important, then mark it with 
MOZ_MAY_IGNORE_RESULT (or similar).

I admit I haven't thoroughly thought through all the implications, maybe it's 
just too hard to put in place and then fix all the errors that would 
immediately pop up, but it'd be interesting to try I think. Or dispiriting.


In the meantime, would you have some suggestions/guidelines as to where we 
should add MOZ_MUST_USE?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Thursday, April 28, 2016 at 1:03:07 PM UTC+10, Ehsan Akhgari wrote:
> [...]
> What about having a MaybeMove function that implementation wise is the
> same as Move, but is exempt from the static check?  That name has the
> advantage of making it clear that a move _may_ happen, but not necessarily.

> On 2016-04-28 9:52 AM, Gerald Squelart wrote:
> > Anyway, how about this:
> > - mozilla::Move() is reserved for expected moves, and *any* re-use after 
> > that is considered an error.
> > - We introduce something like mozilla::MakeAvailableForMove(), which allows 
> > for re-use.
> > - For extra safety, we could allow MakeAvailableForMove() to only work on 
> > classes that have a special attribute, e.g. MOZ_TYPE_IS_REUSABLE_AFTER_MOVE.
> 
> What's the semantics of MakeAvailableForMove()?  Something similar to my
> MaybeMove suggestion?

Exactly the same! I.e., the same implementation as plain Move, but static 
analyzers won't complain.


Anyway, reading some ideas, it may be true that there are always other ways to 
implement optional moves (l-value ref, etc.).
So maybe we should just go ahead with preventing Move'd object reuse for now, 
as it's the safest option. And in the future if someone sees a really 
compelling case that cannot be done as effectively otherwise, they can work on 
an unchecked way to optionally-move objects.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Thursday, April 28, 2016 at 11:25:38 AM UTC+10, Ehsan Akhgari wrote:
> On 2016-04-28 9:00 AM, Gerald Squelart wrote:
> > On Thursday, April 28, 2016 at 10:41:21 AM UTC+10, Ehsan Akhgari wrote:
> >> On 2016-04-28 8:00 AM, Gerald Squelart wrote:
> >>> On Thursday, April 28, 2016 at 9:35:56 AM UTC+10, Kyle Huey wrote:
> >>>> Can we catch this pattern with a compiler somehow?
> >>>>
> >>>> Foo foo;
> >>>> foo.x = thing;
> >>>> DoBar(mozilla::Move(foo));
> >>>> if (foo.x) { /* do stuff */ }
> >>
> >> I think so.  We already have an analysis which would detect whether the
> >> return value of a function is used somewhere or not.  We should be able
> >> to reuse that to find the call to DoBar(), and then look for future
> >> occurrences of foo used as an rvalue in the rest of the function.  Once
> >> we detect a use of "foo" as an lvalue, further usages of it as an rvalue
> >> in the same function should be OK and not trigger the error.  File a bug?
> >>
> >>> Definitely something that would be nice.
> >>>
> >>> But if we have/implement such a catcher, I'd like to have an annotation 
> >>> to say "yep I really want to reuse this moved-from object".
> >>> Because sometimes the function will choose not to actually move from an 
> >>> rvalue-ref, or the object knows to revert to a fully-reusable state, etc.
> >>
> >> What you're describing sounds like a violation of move semantics, right?
> >>  The first case should only happen if DoBar doesn't accept an rvalue
> >> reference, in which case the code above is definitely doing something
> >> that the author did not expect, given that they have used Move().  The
> >> latter case sounds completely broken, and if there is an actual good use
> >> case for it, the C++ move semantics sound like the wrong tool to achieve
> >> that goal to me.
> >>
> >> If you feel like I'm missing something or you can make a strong argument
> >> on why breaking move semantics is OK in some cases, please let me know.  
> >> :-)
> >>
> >> Cheers,
> >> Ehsan
> > 
> > std::move and mozilla:Move are just casts that make an l-value object 
> > *look* like an r-value, so that when the compiler considers which 'DoBar' 
> > to use, one that takes an r-value reference will be picked first.
> > 
> > "Move" is probably not the best name because it gives this impression that 
> > an actual move happens, but that's what we're stuck with in the standard.
> 
> Yes, I understand that.
> 
> > I don't see a "violation of move semantics" in there, could you please 
> > elaborate on what exact move semantics are violated?
> > I'd say it's probably more a "perversion of the move spirit". :-)
> 
> Even though Move() doesn't "actually" move anything, it has a meaning to
> the author: "when I have an lvalue object, I want to move it to some
> other place".
> 
> While it's true that if DoBar() doesn't really move its argument
> somewhere the code compiles, in that case DoBar() is breaking the
> expectation of the caller since the caller clearly intended for a move
> to happen.  In C++ there is no way for the author to know that a move
> actually happened, so the semantics of using the object after the move
> are fuzzy.  This is not really helpful since there is no clear way to
> "check" whether a move actually happened, and it's not quite clear how
> the caller should act if the callee decides to not move.  IOW, without
> something enforcing that a move actually happens, it is impossible to
> understand the code Kyle posted above without knowing the implementation
> of DoBar(), which is a problem with the C++ move semantics.
> 
> However if we change the contract so that the callee is expected to
> always perform a move, then we can perform a useful check on the caller
> side to enforce not touching the object after it has been moved, which
> is what Kyle was asking for.
> 
> > In any case, a moved-from object *must* stay valid after that call, because 
> > at the minimum it will be destroyed at the end of its enclosing scope, by 
> > invoking its normal destructor, no magic or special path there.
> 
> Define valid.  The object will not be *destroyed* after a move, but it
> should be "semantically empty" (the definition of "semantically empty"
> is different depending on the class of the object.)
> 
> I can see how it could be useful to for example call 

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Thursday, April 28, 2016 at 11:00:12 AM UTC+10, Gerald Squelart wrote:
> On Thursday, April 28, 2016 at 10:41:21 AM UTC+10, Ehsan Akhgari wrote:
> > On 2016-04-28 8:00 AM, Gerald Squelart wrote:
> > > On Thursday, April 28, 2016 at 9:35:56 AM UTC+10, Kyle Huey wrote:
> > >> Can we catch this pattern with a compiler somehow?
> > >>
> > >> Foo foo;
> > >> foo.x = thing;
> > >> DoBar(mozilla::Move(foo));
> > >> if (foo.x) { /* do stuff */ }
> > 
> > I think so.  We already have an analysis which would detect whether the
> > return value of a function is used somewhere or not.  We should be able
> > to reuse that to find the call to DoBar(), and then look for future
> > occurrences of foo used as an rvalue in the rest of the function.  Once
> > we detect a use of "foo" as an lvalue, further usages of it as an rvalue
> > in the same function should be OK and not trigger the error.  File a bug?
> > 
> > > Definitely something that would be nice.
> > > 
> > > But if we have/implement such a catcher, I'd like to have an annotation 
> > > to say "yep I really want to reuse this moved-from object".
> > > Because sometimes the function will choose not to actually move from an 
> > > rvalue-ref, or the object knows to revert to a fully-reusable state, etc.
> > 
> > What you're describing sounds like a violation of move semantics, right?
> >  The first case should only happen if DoBar doesn't accept an rvalue
> > reference, in which case the code above is definitely doing something
> > that the author did not expect, given that they have used Move().  The
> > latter case sounds completely broken, and if there is an actual good use
> > case for it, the C++ move semantics sound like the wrong tool to achieve
> > that goal to me.
> > 
> > If you feel like I'm missing something or you can make a strong argument
> > on why breaking move semantics is OK in some cases, please let me know.  :-)
> > 
> > Cheers,
> > Ehsan
> 
> std::move and mozilla:Move are just casts that make an l-value object *look* 
> like an r-value, so that when the compiler considers which 'DoBar' to use, 
> one that takes an r-value reference will be picked first.
> 
> "Move" is probably not the best name because it gives this impression that an 
> actual move happens, but that's what we're stuck with in the standard.
> 
> I don't see a "violation of move semantics" in there, could you please 
> elaborate on what exact move semantics are violated?
> I'd say it's probably more a "perversion of the move spirit". :-)
> 
> In any case, a moved-from object *must* stay valid after that call, because 
> at the minimum it will be destroyed at the end of its enclosing scope, by 
> invoking its normal destructor, no magic or special path there.
> 
> Now what to do with a moved-from object, is I think more a philosophical 
> question! Some argue that we should do nothing (except the inevitable implied 
> destruction). Others think it should be fine to also allow re-assignment 
> (i.e. reuse the variable for something completely different). And yet others 
> would allow total reuse.
> 
> My position is that 'Move(x)' *usually* means we give the value away and 
> don't want to use it again, and therefore a compiler warning/error would help 
> catch unexpected reuses; but also that some situations call for reuse (e.g. 
> the function doesn't always steal the object's contents, based on other 
> factors) and a programmer in the know should be allowed to annotate this 
> special case.

Note that we talked a bit about this situation in:
https://groups.google.com/d/topic/mozilla.dev.platform/VLtl2yL_TlA/discussion
Referring to:
http://mxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h#183
Which talks about conditionally moving from a UniquePtr.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Thursday, April 28, 2016 at 10:41:21 AM UTC+10, Ehsan Akhgari wrote:
> On 2016-04-28 8:00 AM, Gerald Squelart wrote:
> > On Thursday, April 28, 2016 at 9:35:56 AM UTC+10, Kyle Huey wrote:
> >> Can we catch this pattern with a compiler somehow?
> >>
> >> Foo foo;
> >> foo.x = thing;
> >> DoBar(mozilla::Move(foo));
> >> if (foo.x) { /* do stuff */ }
> 
> I think so.  We already have an analysis which would detect whether the
> return value of a function is used somewhere or not.  We should be able
> to reuse that to find the call to DoBar(), and then look for future
> occurrences of foo used as an rvalue in the rest of the function.  Once
> we detect a use of "foo" as an lvalue, further usages of it as an rvalue
> in the same function should be OK and not trigger the error.  File a bug?
> 
> > Definitely something that would be nice.
> > 
> > But if we have/implement such a catcher, I'd like to have an annotation to 
> > say "yep I really want to reuse this moved-from object".
> > Because sometimes the function will choose not to actually move from an 
> > rvalue-ref, or the object knows to revert to a fully-reusable state, etc.
> 
> What you're describing sounds like a violation of move semantics, right?
>  The first case should only happen if DoBar doesn't accept an rvalue
> reference, in which case the code above is definitely doing something
> that the author did not expect, given that they have used Move().  The
> latter case sounds completely broken, and if there is an actual good use
> case for it, the C++ move semantics sound like the wrong tool to achieve
> that goal to me.
> 
> If you feel like I'm missing something or you can make a strong argument
> on why breaking move semantics is OK in some cases, please let me know.  :-)
> 
> Cheers,
> Ehsan

std::move and mozilla:Move are just casts that make an l-value object *look* 
like an r-value, so that when the compiler considers which 'DoBar' to use, one 
that takes an r-value reference will be picked first.

"Move" is probably not the best name because it gives this impression that an 
actual move happens, but that's what we're stuck with in the standard.

I don't see a "violation of move semantics" in there, could you please 
elaborate on what exact move semantics are violated?
I'd say it's probably more a "perversion of the move spirit". :-)

In any case, a moved-from object *must* stay valid after that call, because at 
the minimum it will be destroyed at the end of its enclosing scope, by invoking 
its normal destructor, no magic or special path there.

Now what to do with a moved-from object, is I think more a philosophical 
question! Some argue that we should do nothing (except the inevitable implied 
destruction). Others think it should be fine to also allow re-assignment (i.e. 
reuse the variable for something completely different). And yet others would 
allow total reuse.

My position is that 'Move(x)' *usually* means we give the value away and don't 
want to use it again, and therefore a compiler warning/error would help catch 
unexpected reuses; but also that some situations call for reuse (e.g. the 
function doesn't always steal the object's contents, based on other factors) 
and a programmer in the know should be allowed to annotate this special case.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Static analysis for "use-after-move"?

[Gerald Squelart]

On Thursday, April 28, 2016 at 9:35:56 AM UTC+10, Kyle Huey wrote:
> Can we catch this pattern with a compiler somehow?
> 
> Foo foo;
> foo.x = thing;
> DoBar(mozilla::Move(foo));
> if (foo.x) { /* do stuff */ }
> 
> - Kyle

Definitely something that would be nice.

But if we have/implement such a catcher, I'd like to have an annotation to say 
"yep I really want to reuse this moved-from object".
Because sometimes the function will choose not to actually move from an 
rvalue-ref, or the object knows to revert to a fully-reusable state, etc.

- Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Automation / Firefox 48 switched to Visual Studio 2015 Update 2

[Gerald Squelart]

On Saturday, April 23, 2016 at 6:25:36 AM UTC+10, Gregory Szorc wrote:
> Visual Studio 2015 Update 2 was released just days after we switched
> automation from VS2013 to VS2015 Update 1. We know we were going to take
> Update 2 eventually and we didn't want to run a Windows toolchain for as
> short as a single release, so we just landed the switch from VS2015 Update
> 1 to Update 2 on inbound. This means we should never ship a Firefox dev
> edition, beta, or release using VS2015 Update 1: we'll go from 2013 to
> 2015u2.
> 
> You may want to take the time to upgrade to VS2015 Update 2 so your local
> builds match automation. If you run the Visual Studio 2015 installer, it
> should auto update an out-of-date installer then prompt you to update
> out-of-date packages. There's a "Tools (1.3.1 and Windows 10 SDK
> (10.0.10586)" update under "Windows and Web Development" -> "Universal
> Windows App Development Tools" *that was released after VS2015 Update 2 was
> initially released*. This contains some minor Windows SDK updates and a
> UCRT rebuild and is the version automation is now running. So even if you
> are running VS2015u2, you may not have this "Tools 1.3.1" update and may
> not match automation. It's probably worth 2 minutes of your time to run the
> installer to double check everything matches.
> 
> Bug 1259782 if anyone has problems.

I tried the installer from the vs website, but it didn't seem to pick up the 
updates.

What worked for me: From Visual Studio itself, go in Tools -> Extensions and 
Updates..., and Update 2 (with tools 1.3.1) is in there.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: use nsresult& outparams in constructors to represent failure

[Gerald Squelart]

On Thursday, April 21, 2016 at 4:33:40 PM UTC+10, Nicholas Nethercote wrote:
> On Thu, Apr 21, 2016 at 4:19 PM, Xidorn Quan  wrote:
> >>
> >> Maybe you're referring to factory methods, like this:
> >>
> >>   static T* T::New();
> >>
> >> which would return null on failure. Such methods can be useful, but
> >> there's two problems. First, they're not applicable to stack-allocated
> >> objects. Second, you still have to do your fallible initialization
> >> *within* the factory method, and so you still have to choose with
> >> either constructor+Init or constructor+outparam, so you still have to
> >> make a choice.
> >
> > You can probably merge Init into this factory method, and make constructors
> > private.
> 
> Inlining Init into the factory method doesn't change the fundamentals.
> Initialization is still split across two functions. You still can't
> use references and |const| as much.
> 
> > For stack-allocated objects, you can probably return a Maybe<>?
> 
> I played around with Maybe<> and couldn't come up with something nice.
> I may have missed something. Even if there is a nice way involving
> Maybe<>, I'm pretty sure you'll still end up having to make a choice
> between constructor+Init and constructor+outparam within the factory
> method.
> 
> Nick

How about another generic helper, e.g.:
  template 
  Maybe MakeCheckedMaybe(Args&&... aArgs)
  {
nsresult rv;
Maybe m = Some(T(std::forward(aArgs)..., ));
if (NS_SUCCEEDED(rv)) {
  return m;
}
return Nothing();
  }

No need for factory methods. ;-)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal: use nsresult& outparams in constructors to represent failure

[Gerald Squelart]

On Thursday, April 21, 2016 at 11:15:10 AM UTC+10, Nicholas Nethercote wrote:
> Hi,
> 
> C++ constructors can't be made fallible without using exceptions. As a result,
> for many classes we have a constructor and a fallible Init() method which must
> be called immediately after construction.
> 
> Except... there is one way to make constructors fallible: use an |nsresult&
> aRv| outparam to communicate possible failure. I propose that we start doing
> this.
> 
> Here's an example showing stack allocation and heap allocation. Currently, we
> do this (boolean return type):
> 
>   T ts();
>   if (!ts.Init()) {
> return NS_ERROR_FAILURE;
>   }
>   T* th = new T();
>   if (!th.Init()) {
> delete th;
> return NS_ERROR_FAILURE;
>   }
> 
> or this (nsresult return type):
> 
>   T ts();
>   nsresult rv = ts.Init();
>   if (NS_FAILED(rv)) {
> return rv;
>   }
>   T* th = new T();
>   rv = th.Init();
>   if (NS_FAILED(rv)) {
> delete th;
> return rv;
>   }
> 
> (In all the examples you could use a smart pointer to avoid the explicit
> |delete|. This doesn't affect my argument in any way.)
> 
> Instead, we would do this:
> 
>   nsresult rv;
>   T ts(rv);
>   if (NS_FAILED(rv)) {
> return rv;
>   }
>   T* th = new T(rv);
>   if (NS_FAILED(rv)) {
> delete th;
> return rv;
>   }
> 
> For constructors with additional argument, I propose that the |nsresult&|
> argument go last.
> 
> Using a bool outparam would be possible some of the time, but I suggest always
> using nsresult for consistency, esp. given that using bool here would be no
> more concise.
> 
> SpiderMonkey is different because (a) its |operator new| is fallible and (b) 
> it
> doesn't use nsresult. So for heap-allocated objects we *would* use bool, going
> from this:
> 
>   T* th = new T();
>   if (!th) {
> return false;
>   }
>   if (!th.Init()) {
> delete th;
> return false;
>   }
> 
> to this:
> 
>   bool ok;
>   T* th = new T(ok);
>   if (!th || !ok) {
> delete th;
> return false;
>   }
> 
> These examples don't show inheritance, but this proposal works out
> straightforwardly in that case.
> 
> The advantages of this proposal are as follows.
> 
> - Construction is atomic. It's not artificially split into two, and there's no
>   creation of half-initialized objects. This tends to make the code nicer
>   overall.
> 
> - Constructors are special because they have initializer lists -- there are
>   things you can do in initializer lists that you cannot do in normal
>   functions. In particular, using an Init() function prevents you from using
>   references and |const| for some members. This is bad because references and
>   |const| are good things that can make code more reliable.
> 
> - There are fewer things to forget at call sites. With our current approach 
> you
>   can forget (a) to call Init(), and (b) to check the result of
> Init(). With this
>   proposal you can only forget to check |rv|.
> 
> The only disadvantage I can see is that it looks a bit strange at first. But 
> if
> we started using it that objection would quickly go away.
> 
> I have some example patches that show what this code pattern looks like in
> practice. See bug 1265626 parts 1 and 4, and bug 1265965 part 1.
> 
> Thoughts?
> 
> Nick

(busy right now, please excuse terseness & typos!)

Big thumbs up for trying to remove split construction

My main beef with this proposal is the use of out-params, which require 
(usually uninitialized) declaration of the out-param.
But I see that it may indeed be the best solution here, so ... fne!

However, since lots of Mozilla objects are unique-ptr'd or ref-counted, I would 
argue that we could easily fold the construction checks with the nullptr-checks 
that we all know!

So in addition to your proposal, I would like to see a small library of tools 
that will build on top of your new style, and make it easier & cleaner & 
consistent to use in those cases.

E.g.:
  template 
  T* newWithCheck(Args&&... aArgs)
  {
nsresult rv;
T* p = new T(std::forward(aArgs)..., );
if (p) { // <- this test could be removed for non-fallible new.
  if (NS_SUCCEEDED(rv)) {
return p;
  }
  delete p; // "Failed" construction -> Just delete the thing.
}
return nullptr;
  }

Then instead of:
  nsresult rv; // Yuck!
  RefPtr foo = new Foo(a, b, ); // Hiss!
  if (NS_SUCCEEDED(rv)) { ... // Can we really trust foo here? Boo!

We could do things like:
  RefPtr foo = newWithCheck(a, b); // Beauty!
  if (foo) { ... // Construction-check & nullptr-check in one test!

Am I showing some bias? :-)

We could have similar 'new' functions that would populate a Maybe instead, 
or that would expose the nsresult where useful (e.g. through Pair), etc.


And of course, there's still the issue of missing checks.

Your style probably helps, as the compiler and/or static analyzers should be 
able to see that you're defining 'rv', writing to it, but not reading it. If 

Re: The convention to pass a parameter when ownership transfer is optional

[Gerald Squelart]

On Friday, March 18, 2016 at 2:07:34 PM UTC+11, jww...@mozilla.com wrote:
> https://hg.mozilla.org/mozilla-central/file/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/mfbt/UniquePtr.h#l182
> 
> "To unconditionally transfer ownership of a UniquePtr into a method, use a 
> |UniquePtr| argument.  To conditionally transfer ownership of a resource into 
> a method, should the method want it, use a |UniquePtr&&| argument."
> 
> Does that also apply to already_AddRefed<>&& or we stick to 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1247972#c19?
> 
> Btw, we have some code like 
> https://hg.mozilla.org/mozilla-central/file/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/dom/base/WebSocket.cpp#l2790
>  where it really should be just already_AddRefed<> since the ownership is 
> always transferred.

One concern I have with foo(T&&) being used to mark optional transfer, is that 
on the caller side there will be a foo(Move(t)) followed by code that may 
*still* uses 't'.
I can't remember exactly where I read/heard that (Herb Sutter I think?), but it 
was advised that moved-from objects should only be destroyed, or completely 
overwritten through an assignment; and this philosophy was used to design STL 
containers move-related APIs. -- Or am I imagining things?


Specific classes (like UniquePtr) might in fact be designed for this particular 
idiom, and I think it's fine when fully documented and tested.

But it'd be great to come to agreement and document it in coding guidelines, 
whether we're going all guns blazing for T&&/Move-and-keep-using, or 
non-recommended-but-allowed-if-you-really-really-know-what-you're-doing, or 
something else entirely...

gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Decommissioning "dumbmake"

[Gerald Squelart]

On Monday, October 19, 2015 at 11:09:46 AM UTC+11, Eric Rescorla wrote:
> On Sun, Oct 18, 2015 at 4:14 PM, Nicholas Nethercote
> wrote:
> 
> > On Sun, Oct 18, 2015 at 3:12 PM, Eric Rescorla wrote:
> > >
> > > What's needed here is a dependency management system that
> > > simply builds what's needed regardless of what's changed,
> >
> > Otherwise known as "a proper build system". glandium and co. have been
> > working towards that for a long time. It's a big, difficult job. |mach
> > build binaries| and |mach build faster| are temporary waypoints along
> > the way that approximate "a proper build system" for a couple of
> > common workflows. Eventually |mach build| should just do the right
> > thing, no matter what files you've touched...
> >
> > > not more ways for the user to tell the build system "only rebuild some
> > stuff".
> >
> > ... except that bholley and ehsan are asking for a way to override the
> > dependency tracking and just rebuild particular directories.
> 
> 
> I don't want to speak for bholley and ehsan, but when *I* run into this
> problem it's because the build system isn't doing the right thing. If it
> were, I wouldn't have to.
> 
> -Ekr

I think the situation they're talking about, and which I've experienced myself, 
is that sneezing at a header file that's included almost everywhere means a 
very long rebuild time -- which is the right thing for the build system to do 
of course!

But if we've only changed a comment, or added a method, or are just trying to 
implement/fix a bug and test a patch quickly in isolation, then we should have 
the ability to locally override the build system decisions.

There are probably ways around that (e.g. experiment in a cpp file before 
moving code to a header), but it's sometimes cumbersome or just not worth the 
effort, compared with rebuilding only parts of the system.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: easily getting allocation stacks for leaking objects

[Gerald Squelart]

Good stuff!

I hope you'll consider tracking AddRef's and Release's as well.

I recently experimented with that for a troubled RefCounted class [1], and it 
was very useful to find which AddRef didn't have its corresponding Release.

Cheers,
Gerald

[1] https://hg.mozilla.org/try/rev/8dffaf0d2acf
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: easily getting allocation stacks for leaking objects

[Gerald Squelart]

On Friday, September 18, 2015 at 3:55:27 AM UTC+10, Andrew McCreight wrote:
> On Thu, Sep 17, 2015 at 7:42 AM, Andrew McCreight
> wrote:
> >
> > On Thu, Sep 17, 2015 at 5:31 AM, Gerald Squelart
> > wrote:
> >
> >> Good stuff!
> >>
> >> I hope you'll consider tracking AddRef's and Release's as well.
> >>
> >> I recently experimented with that for a troubled RefCounted class [1],
> >> and it was very useful to find which AddRef didn't have its corresponding
> >> Release.
> >
> > There's already refcount logging for this:
> >   http://www-archive.mozilla.org/performance/refcnt-balancer.html
> >
> Olli points out that I should actually link to the more modern page for
> this:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Refcount_tracing_and_balancing
> 
> Anybody who wants help with refcount logging should feel free to get in
> touch with me and I can help walk them through it.
> 
> 
> > It is generally too spammy to include in the TreeHerder log, but it can be
> > useful.
> >
> > Andrew

Thanks for the links Andrew, I'll give it a go next time!
g.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Proposal to alter the coding style to not require the usage of 'virtual' where 'override' is used

[Gerald Squelart]

Will there be a blanket change sometime soon?

If not, are we supposed to start using the new style in patches now, even when 
it would clash with nearby old-style overrides/finals in the same file or in 
the same directory?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform