Re: C++ method definition comments

[gsquelart]

On Wednesday, January 30, 2019 at 9:57:02 AM UTC+11, Ehsan Akhgari wrote:
> On Mon, Jan 28, 2019 at 7:10 PM  wrote:
> 
> > Just a thought: Would it be worth considering a blank macro, e.g.:
> > static void foo();
> > DECLARED_STATIC void foo() {...}
> >
> > On top of not being confused with other comments around, it could be
> > clang-checked so it's never wrong. (And maybe eventually enforced, like
> > MOZ_IMPLICIT is.)
> >
> 
> Yeah, that could be a future alternative, but it would require someone to
> do the hard work of implementing the required static analysis and landing
> it.  I think Ryan's proposal will probably simplify that process somewhat
> by making it possible to mass-replace a bunch of "// static" comments with
> "DECLARED_STATIC" or some such, but I don't want to hold the good solution
> here for a perfect solution in the future.  I would personally be OK for
> these two to happen incrementally.
> 
> Would you mind filing a bug for this please?
> 
> Thanks,
> Ehsan

Thank you Ehsan. Yes, a phased approach would definitely be the way to go.
https://bugzilla.mozilla.org/show_bug.cgi?id=1523793

I realize now that this doesn't help at all with the issue of valuable 
horizontal real-estate! Sorry for the tangent.

One (partly tongue-in-cheek) solution to make the important function name more 
prominent is to use trailing return types:
`auto Foo::Bar() -> void {`
Note that this is more easily greppable when searching for function names.
And it looks more like Rust.

To help with spacing, the `DECLARED_...` macros could be placed at the end (if 
possible?) :
`void Foo::Bar() DECLARED_STATIC {`
`auto Foo::Bar() -> void DECLARED_STATIC {`
But this feels uglier to me.

Cheers,
Gerald

> > On Tuesday, January 29, 2019 at 10:27:17 AM UTC+11, Ryan Hunt wrote:
> > > Yeah, personally I have found them be useful and don't have an issue
> > with keeping
> > > them. I just wasn't sure if that was a common experience.
> > >
> > > So for converting from C-style to C++-style, that would be:
> > >
> > > /* static */ void Foo::Bar() {
> > >  ...
> > > }
> > >
> > > // static
> > > void Foo::Bar() {
> > >  ...
> > > }
> > >
> > > I think that would be good. My one concern would be the presence of
> > other C++-style
> > > comments before the method definition. For example [1].
> > >
> > > Ideally documentation like that should go in the header by the method
> > declaration, but I
> > > have no idea if we consistently do that.
> > >
> > > [1]
> > https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1023
> > >
> > > Thanks,
> > > Ryan
> > >
> > > ‐‐‐ Original Message ‐‐‐
> > > On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari 
> > wrote:
> > >
> > > > This is indeed one of the cases where the reformat has made things
> > worse.  I think as a couple of people have already said, we'll find that
> > some people do find these annotations useful, even if they're not always
> > consistently present.
> > > >
> > > > The path to least resistance for addressing this problem may be to
> > convert these into C++-style comments and therefore moving them into their
> > own lines.  Would you be OK with that?
> > > >
> > > > Thanks,
> > > > Ehsan
> > > >
> > > > On Fri, Jan 25, 2019 at 11:49 PM Ryan Hunt  wrote:
> > > >
> > > >> Hi all,
> > > >>
> > > >> Quick C++ style question.
> > > >>
> > > >> A common pattern in Gecko is for method definitions to have a comment
> > with the
> > > >> 'static' or 'virtual' qualification.
> > > >>
> > > >> Before the reformat, the comment would be on it's own separate line
> > [1]. Now
> > > >> it's on the main line of the definition [2].
> > > >>
> > > >> For example:
> > > >>
> > > >> /* static */ void
> > > >> Foo::Bar() {
> > > >>   ...
> > > >> }
> > > >>
> > > >> vs.
> > > >>
> > > >> /* static */ void Foo::Bar() {
> > > >>   ...
> > > >> }
> > > >>
> > > >> Personally I think this now takes too much horizontal space from the
> > main
> > > >> definition, and would prefer it to be either on its own line or just
> > removed.
> > > >>
> > > >> Does anyone have an opinion on whether we still want these comments?
> > And if so
> > > >> whether it makes sense to move them back to their own line.
> > > >>
> > > >> (My ulterior motive is that sublime text's indexer started failing to
> > find
> > > >>  these definitions after the reformat, but that should be fixed
> > regardless)
> > > >>
> > > >> If you're interested in what removing these would entail, I wrote a
> > regex to
> > > >> make the change [3].
> > > >>
> > > >> Thanks,
> > > >> Ryan
> > > >>
> > > >> [1]
> > https://hg.mozilla.org/mozilla-central/file/0348d472115d/layout/generic/nsFrame.cpp#l1759
> > > >> [2]
> > https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1756
> > > >> [3]
> > https://hg.mozilla.org/try/rev/31ab3e466b6f15dcdbb1aee47edabc7c358b86f2
> > > >>
> > > >
> > > > --
> > > > Ehsan
> >
> 
> 
> -- 
> Ehsan

Re: C++ method definition comments

[gsquelart]

Just a thought: Would it be worth considering a blank macro, e.g.:
static void foo();
DECLARED_STATIC void foo() {...}

On top of not being confused with other comments around, it could be 
clang-checked so it's never wrong. (And maybe eventually enforced, like 
MOZ_IMPLICIT is.)

Cheers,
Gerald

On Tuesday, January 29, 2019 at 10:27:17 AM UTC+11, Ryan Hunt wrote:
> Yeah, personally I have found them be useful and don't have an issue with 
> keeping
> them. I just wasn't sure if that was a common experience.
> 
> So for converting from C-style to C++-style, that would be:
> 
> /* static */ void Foo::Bar() {
>  ...
> }
> 
> // static
> void Foo::Bar() {
>  ...
> }
> 
> I think that would be good. My one concern would be the presence of other 
> C++-style
> comments before the method definition. For example [1].
> 
> Ideally documentation like that should go in the header by the method 
> declaration, but I
> have no idea if we consistently do that.
> 
> [1] 
> https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1023
> 
> Thanks,
> Ryan
> 
> ‐‐‐ Original Message ‐‐‐
> On Monday, January 28, 2019 12:51 PM, Ehsan Akhgari  
> wrote:
> 
> > This is indeed one of the cases where the reformat has made things worse.  
> > I think as a couple of people have already said, we'll find that some 
> > people do find these annotations useful, even if they're not always 
> > consistently present.
> >
> > The path to least resistance for addressing this problem may be to convert 
> > these into C++-style comments and therefore moving them into their own 
> > lines.  Would you be OK with that?
> >
> > Thanks,
> > Ehsan
> >
> > On Fri, Jan 25, 2019 at 11:49 PM Ryan Hunt  wrote:
> >
> >> Hi all,
> >>
> >> Quick C++ style question.
> >>
> >> A common pattern in Gecko is for method definitions to have a comment with 
> >> the
> >> 'static' or 'virtual' qualification.
> >>
> >> Before the reformat, the comment would be on it's own separate line [1]. 
> >> Now
> >> it's on the main line of the definition [2].
> >>
> >> For example:
> >>
> >> /* static */ void
> >> Foo::Bar() {
> >>   ...
> >> }
> >>
> >> vs.
> >>
> >> /* static */ void Foo::Bar() {
> >>   ...
> >> }
> >>
> >> Personally I think this now takes too much horizontal space from the main
> >> definition, and would prefer it to be either on its own line or just 
> >> removed.
> >>
> >> Does anyone have an opinion on whether we still want these comments? And 
> >> if so
> >> whether it makes sense to move them back to their own line.
> >>
> >> (My ulterior motive is that sublime text's indexer started failing to find
> >>  these definitions after the reformat, but that should be fixed regardless)
> >>
> >> If you're interested in what removing these would entail, I wrote a regex 
> >> to
> >> make the change [3].
> >>
> >> Thanks,
> >> Ryan
> >>
> >> [1] 
> >> https://hg.mozilla.org/mozilla-central/file/0348d472115d/layout/generic/nsFrame.cpp#l1759
> >> [2] 
> >> https://hg.mozilla.org/mozilla-central/file/e4b9b1084292/layout/generic/nsFrame.cpp#l1756
> >> [3] https://hg.mozilla.org/try/rev/31ab3e466b6f15dcdbb1aee47edabc7c358b86f2
> >>
> >
> > --
> > Ehsan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming C++ standards meeting in San Diego, California

[gsquelart]

On Tuesday, October 23, 2018 at 10:57:59 AM UTC+11, Botond Ballo wrote:
> On Mon, Oct 22, 2018 at 7:47 PM,   wrote:
> > Here's one I'd like:
> >   for (int i = ...; test(i); i = next(i)) {
> > if (good_stuff(i)) { break; /* goto past `else` block */ }
> >   } else {
> > /* test(i) failed, i.e., we didn't `break` */
> > do_something(i); // decls inside `for(...)` still in scope.
> >   }
> > and/or:
> >   for (auto& x: xs) {
> > if (good_stuff(x)) { break; /* goto past `else` block */ }
> >   } else {
> > /* went off-range, i.e., we didn't `break` */
> >   }
> 
> This has actually been proposed before! Here's the latest iteration of
> the proposal (dated February 2017):
> 
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0082r2.pdf
> 
> and here's my summary of its discussion, from my blog post about the
> meeting where it was discussed [1]:
> 
> "An updated version of for loop exit strategies, which proposes to
> allow adding blocks of code at the end of a for loop that run when the
> loop exits by breaking (introduced by the keyword on_break), and when
> the loop runs to completion (introduced by on_complete). The only
> thing that has changed since the last revision of this proposal is the
> keywords – they were catch break and catch default in the previous
> version. EWG didn’t love the new keywords, either, and was generally
> lukewarm towards the proposal in terms of motivation, so it’s not
> clear if this is going anywhere."
> 
> Given that this has already been proposed and effectively shot down, I
> think it makes to try and re-propose it only if we have new
> information to bring to the table, e.g. compelling new motivation or a
> design alternative that hasn't been considered.
> 
> Cheers,
> Botond
> 
> 
> [1] 
> https://botondballo.wordpress.com/2017/03/27/trip-report-c-standards-meeting-in-kona-february-2017/

Thanks for the info.
I guess it's quite a common need, it must have been on many minds since 1972. 
:-)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Upcoming C++ standards meeting in San Diego, California

[gsquelart]

On Tuesday, October 23, 2018 at 8:52:47 AM UTC+11, Botond Ballo wrote:
> I will be attending this meeting

So jealous! Enjoy ;-)

> Finally, I encourage you to reach out to me if you're thinking of
> submitting a proposal to the committee. I'm always happy to help with
> formulating and, if necessary, presenting a proposal.

Here's one I'd like:
  for (int i = ...; test(i); i = next(i)) {
if (good_stuff(i)) { break; /* goto past `else` block */ }
  } else {
/* test(i) failed, i.e., we didn't `break` */
do_something(i); // decls inside `for(...)` still in scope.
  }
and/or:
  for (auto& x: xs) {
if (good_stuff(x)) { break; /* goto past `else` block */ }
  } else {
/* went off-range, i.e., we didn't `break` */
  }


Thanks in advance! :-D
(Though seriously: Worth pursuing?)

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

Re: Plan for Sunsetting MozReview

[gsquelart]

(Disclaimer: I'm not from IT!)

Until this gets fixed, a workaround for closed bugs is to go to the bottom of 
the bug, and look for https://hg.mozilla.org/mozilla-central/rev/... links.
Not as pretty, and missing review context, but hopefully this should help 
explore the changed code in most cases.

Cheers,
Gerald

On Tuesday, August 28, 2018 at 8:17:24 AM UTC+10, Eric Shepherd (Sheppy) wrote:
> We've noticed that attachment links are no longer working because they're
> still trying to go to reviewboard, and there don't appear to be redirects.
> See for example this bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1211330. It has two
> attachments. Clicking either one of them gives you a hard-hat page instead
> of the changes.
> 
> Eric Shepherd
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Fission MemShrink Newsletter #1: What (it is) and Why (it matters to you)

[gsquelart]

On Wednesday, July 11, 2018 at 4:19:15 AM UTC+10, Kris Maglione wrote:
> [...]
> Essentially what this means, though, is that if we identify an area of 
> overhead that's 50KB[3] or larger that can be eliminated, it *has* to be 
> eliminated. There just aren't that many large chunks to remove. They all need 
> to go. And if an area of code has a dozen 5KB chunks that can be eliminated, 
> maybe they don't all have to go, but at least half of them do. The more the 
> better.

Some questions: -- Sorry if some of this is already common knowledge or has 
been discussed.

Are there tools available, that could easily track memory usage of specific 
things?
E.g., could I instrument one class, so that every allocation would be tracked 
automatically, and I'd get nice stats at the end?
Including wasted space because of larger allocation blocks?

Could I even run what-if scenarios, where I could instrument a class and 
extract its current size but also provide an alternate size (based on what I 
think I could make it shrink), and in the end I'll know how much I could save 
overall?

Do we have Try tests that simulate real-world usage, so we could collect 
memory-usage data that's relevant to our users, but also reproducible?

Should there be some kind of Talos-like CI tests that focus on memory usage, so 
we'd get some warning if a particular patch suddenly eats too much memory?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style: brace initialization syntax

[gsquelart]

On Wednesday, June 6, 2018 at 5:35:59 AM UTC+10, Boris Zbarsky wrote:
> On 6/5/18 3:10 PM, Emilio Cobos Álvarez wrote:
> > I personally would prefer one space at each side when using braces:
> > 
> >   , mFoo { 0 }
> 
> I think the reason people tend to think of this as not wanting spaces is 
> that they are thinking of it as a constructor call.  The parentheses 
> syntax for initializer lists helps think of things that way, of course.
> [...]
> -Boris

I also prefer `m{ 0 }`.

`m(0)` (direct initialization) and `m{ 0 }` (list initialization) are really 
different operations, and may in fact call different constructors -- E.g., the 
latter will prefer constructors that take an std::initializer_list.
So I think it is important to emphasize the difference, especially for 
reviewers to be able to pick on that difference.

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

Re: PSA: Building Firefox 61+ with GCC will soon require version GCC 6.1+

[gsquelart]

Good stuff, thanks Jeff!

To be pedantic, don't you mean "start relying on *more* c++14 features"?

Because we've already been able to use some C++14 features since November 2017 
(see bug 1325632, landed in 59).
E.g., I and others have used initialized lambda captures since then -- saving 
quite a bit of boilerplate when capturing RefPtr's, and allowing capture of 
move-only things like UniquePtr.

Gerald


On Friday, April 6, 2018 at 1:00:03 AM UTC+10, Jeff Gilbert wrote:
> Bug 1444274 will bump our minimum GCC version to 6.1. GCC-7 will
> continue to work.
> 
> If you build with GCC instead of Clang on Linux, I've been told that
> the system gcc package for Ubuntu 16.04 LTS is gcc-5, so very soon
> you'll need to install a gcc-6 package to continue to build.
> 
> With a bump to GCC 6, along with the bump to vs2017, we should be able
> to start relying on c++14 features. This will help out with tracking
> upstream projects which are starting to rely in c++14, as well as
> letting us phase in new tools into our c++ toolbox. (relaxed constexpr
> is a big one)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Hiding 'new' statements - Good or Evil?

[gsquelart]

Should we allow hiding 'new' statements, or keep them as visible as possible?


Some context:
Recently in bug 1410252 I added a MakeNotNull(args...) function that does 
`NotNull(new T(args...))`, in the style of MakeUnique and others. It also 
works with RefPtr.

My first goal was to avoid the unnecessary nullptr-check in the NotNull 
constructor, since our new is infallible by default.

And I thought that hiding the naked new statement was a nice side benefit, as I 
was under the impression that it was A Good Thing in modern C++. (Though I 
think the main reason for that, was to prevent leaks when handling exceptions 
in multi-allocation expressions, so it doesn't apply to us here.)

Now, a colleague remarked that this was making the memory allocation less 
obvious.
"What about MakeUnique?" I asked; "I'm used to them now" he retorted. :-P


So, what do you all think?
- Should I remove MakeNotNull?
- Or should we allow/encourage more MakeX functions instead of X(new...)? I'm 
thinking MakeRefPtr might be nice.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: mozilla-central now compiles with C++14

[gsquelart]

On Friday, November 17, 2017 at 6:05:38 AM UTC+11, Jeff Walden wrote:
> On 11/16/2017 08:35 AM, Ben Kelly wrote:
> > I would like to use initialized lambda capture as I think it will allow
> > move-only objects to be used in lambdas like this:
> > 
> > UniquePtr uniqueThing = MakeUnique();
> > nsCOMPtr r = NS_NewRunnableFunction([uniqueThing =
> > Move(uniqueThing)] () {
> >   uniqueThing->Stuff();
> > });
> > thread->Dispatch(r.forget());
> > 
> > Thanks.
> 
> Do it!  https://bugzilla.mozilla.org/show_bug.cgi?id=1322962 was WONTFIX'd 
> for adding a dodgy workaround for this purpose, so it is expected that people 
> will do/use this in this manner.
> 
> Jeff

Please note one hurdle before C++14 can actually be used:
https://bugzilla.mozilla.org/show_bug.cgi?id=1418047

This should hopefully land soon.
Then let the floodgates open, and let us yearn for C++17 features we suddenly 
*need* right now! :-)

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

Re: Are char* and uint8_t* interchangeable?

[gsquelart]

On Thursday, November 9, 2017 at 1:11:20 PM UTC+11, Kris Maglione wrote:
> On Wed, Nov 08, 2017 at 06:04:27PM -0800, jww...@mozilla.com wrote:
> >Is it always safe and portable to do:
> >
> >char* p1 = ...;
> >uint8_t* p2 = reinterpret_cast(p1);
> >uint8_t u8 = p2[0];
> >
> >without breaking strict aliasing?
> 
> Strict aliasing permits any typed data to be accessed as char*, 
> so yes, this is always safe and portable. Though they aren't 
> strictly interchangeable.

Kris, if you look at the code sample, it's doing the reverse: Accessing char* 
data as uint8_t*. Is *that* safe?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style: `else for` or `else { for... }`?

[gsquelart]

Only negative reactions to `else for` so far, and it's only used a couple of 
times in our code:
http://searchfox.org/mozilla-central/search?q=else%5B+%5D%2B(do%7Cfor%7Cswitch%7Cwhile)%5B+%5D=true=true=*.cpp

So I'll add a clarification in the coding style page, that `else` should only 
be followed by `{` or `if`.

On Thursday, August 31, 2017 at 10:41:41 AM UTC+12, Jeff Gilbert wrote:
> IMO: Never else-for. (or else-while)
> 
> Else-if is a reasonable continuation of concept: "Well it wasn't that,
> what if it's this instead?"
> Else-for is just shorthand for "well it wasn't that, so let's loop
> over something".
> 
> Else-if is generally used for chaining, often effectively as an
> advanced switch/match statement.
> 
> I've never actually seen else-for, but I imagine it would be used for:
> if (!foo) {
> } else for (i = 0; i < foo->bar; i++) {
> }
> 
> I don't think this pattern has enough value to be acceptable as
> shorthand. BUT, I didn't know it was a thing until now, so I'm
> naturally biased against it.
> 
> On Wed, Aug 30, 2017 at 2:58 PM,   wrote:
> > Let's keep the flames alive!
> >
> > Should we always put braces after an `else`, with the only exception being 
> > another `if`?
> > Or should we also have exceptions for the other control structures (while, 
> > do, for, switch)?
> >
> > A.
> > if (...) {
> >   ...
> > } else {
> >   for (...) {
> > ...
> >   }
> > }
> >
> > B.
> > if (...) {
> >   ...
> > } else for (...) {
> >   ...
> > }
> >
> > I can see arguments for both, so I'm not too sure which one should 
> > win. :-)
> > WDYT?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Coding style: `else for` or `else { for... }`?

[gsquelart]

Let's keep the flames alive!

Should we always put braces after an `else`, with the only exception being 
another `if`?
Or should we also have exceptions for the other control structures (while, do, 
for, switch)?

A.
if (...) {
  ...
} else {
  for (...) {
...
  }
}

B.
if (...) {
  ...
} else for (...) {
  ...
}

I can see arguments for both, so I'm not too sure which one should win. 
:-)
WDYT?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Extensions and Gecko specific APIs

[gsquelart]

On Wednesday, July 26, 2017 at 8:21:23 AM UTC+12, Andrew Overholt wrote:
> On Tue, Jul 25, 2017 at 3:06 PM, David Teller  wrote:
> 
> > Should we moz-prefix moz-specific extensions?
> 
> 
> We have been trying not to do that for Web-exposed APIs but maybe the
> extensions case is different?
> https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Guiding_Principles

Thank you Olli for starting this discussion.
For some context: Olli reviewed my initial attempt to bring some [chrome-only] 
moz-specific extensions for use in the legacy about:media extension, to a 
privilege-gated API for use in a (Web)Extension rewrite, to make it work at 
least as well as the old one.


So in my case, I'd like to get some debugging data from a  element, and 
build a useful DevTools panel from that. (Currently we have APIs like 
HTMLMediaElement.mozRequestDebugInfo() to get such data, but only extensions 
with the "debugger" or "tabs" privilege can see them; so any random webdev 
won't see these things.)

The collected data will be a kind of log that captures the changing state of 
our Media Playback stack and how data goes through it; a bit similar to what an 
unnamed competitor already does in their 'chrome://media-internals/' page -- 
but much better of course!

I think that such an API could be spec'd such that it is portable, with the 
output being flexible enough that we can put Mozilla-specific information in 
there. E.g.: A fixed API to get the data, and a minimal structure for the 
output, but say, each log message could have a free-form 'data' json object, or 
some self-described tabular data.

But with no experience in producing cross-browser specs, I'm scared that such a 
process would be time-consuming(?)

So in the interest of producing a useful Media DevTools Panel soon-ish I'm 
thinking of continuing with moz-specific APIs for now, as development 
progresses and I tweak these APIs...
But with the view of eventually converting them to a proper WebExtension API.
Of course the risk there is that once we have a working DevTools panel, there 
may be little incentive to Do The Right Thing and work on proper specs.


What do you all think? (in general and/or for my specific case)

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

Re: Improving visibility of compiler warnings

[gsquelart]

On Friday, May 26, 2017 at 11:08:09 AM UTC+12, Andrew McCreight wrote:
> On Thu, May 25, 2017 at 4:03 PM,  wrote:
> 
> > On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote:
> > > On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:
> > > > On 05/19/2017 02:44 PM, Gregory Szorc wrote:
> > > >> `mach build` attempts to parse compiler warnings to a persisted
> > > >> "database."
> > > >> You can view a list of compiler warnings post build by running `mach
> > > >> warnings-list`. The intent behind this feature was to make it easier
> > to
> > > >> find and fix compiler warnings. After all, something out of sight is
> > > >> out of
> > > >> mind.
> > >
> > > Given that we treat warnings as errors on CI and most directories that
> > > opt out of warnings-as-errors are third-party code
> > > (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to
> > > make the warning list any more visible. A warning regression in nearly
> > > all first-party code is already treated as an error.
> > >
> > >
> > > [1]
> > > https://searchfox.org/mozilla-central/search?q=ALLOW_
> > COMPILER_WARNINGS=true=moz.build
> >
> > Tangentially: "we treat warnings as errors on CI" -- It'd be great if
> > local builds could have that behavior by default, so we (or just I?) don't
> > get caught by surprise when warnings-as-errors appear after landing.
> >
> > Or at least: Can I opt-in locally? (I tried 'ac_add_options
> > --enable-warnings-ar-errors' in mozconfig, but that was not recognized.)
> >
> 
> This has worked for me before:
> ac_add_options --enable-warnings-as-errors

Haha, I've just noticed that I had typed '--enable-warnings-ar-errors' (Notice 
the 'ar' instead of 'as' -- must have been pirate day!)

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

Re: Improving visibility of compiler warnings

[gsquelart]

On Friday, May 26, 2017 at 6:03:02 AM UTC+12, Chris Peterson wrote:
> On 2017-05-25 5:31 AM, Ehsan Akhgari wrote:
> > On 05/19/2017 02:44 PM, Gregory Szorc wrote:
> >> `mach build` attempts to parse compiler warnings to a persisted
> >> "database."
> >> You can view a list of compiler warnings post build by running `mach
> >> warnings-list`. The intent behind this feature was to make it easier to
> >> find and fix compiler warnings. After all, something out of sight is
> >> out of
> >> mind.
> 
> Given that we treat warnings as errors on CI and most directories that 
> opt out of warnings-as-errors are third-party code 
> (ALLOW_COMPILER_WARNINGS in moz.build [1]), I don't think we need to 
> make the warning list any more visible. A warning regression in nearly 
> all first-party code is already treated as an error.
> 
> 
> [1] 
> https://searchfox.org/mozilla-central/search?q=ALLOW_COMPILER_WARNINGS=true=moz.build

Tangentially: "we treat warnings as errors on CI" -- It'd be great if local 
builds could have that behavior by default, so we (or just I?) don't get caught 
by surprise when warnings-as-errors appear after landing.

Or at least: Can I opt-in locally? (I tried 'ac_add_options 
--enable-warnings-ar-errors' in mozconfig, but that was not recognized.)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about commit messages: they should be useful

[gsquelart]

On Tuesday, April 18, 2017 at 11:58:11 AM UTC+12, smaug wrote:
> On 04/18/2017 02:36 AM, Gregory Szorc wrote:
> > On Mon, Apr 17, 2017 at 4:10 PM, smaug  wrote:
> >
> >> On 04/17/2017 06:16 PM, Boris Zbarsky wrote:
> >>
> >>> A quick reminder to patch authors and reviewers.
> >>>
> >>> Changesets should have commit messages.  The commit message should
> >>> describe not just the "what" of the change but also the "why".  This is
> >>> especially
> >>> true in cases when the "what" is obvious from the diff anyway; for larger
> >>> changes it makes sense to have a summary of the "what" in the commit
> >>> message.
> >>>
> >>> As a specific example, if your diff is a one-line change that changes a
> >>> method call argument from "true" to "false", having a commit message that
> >>> says
> >>> "change argument to mymethod from true to false" is not very helpful at
> >>> all.  A good commit message in this situation will at least mention the
> >>> meaning for the argument.  If that does not make it clear why the change
> >>> is being made, the commit message should explain the "why".
> >>>
> >>> Thank you,
> >>> Boris
> >>>
> >>> P.S.  Yes, this was prompted by a specific changeset I saw.  This
> >>> changeset had been marked r+, which means neither the patch author not the
> >>> reviewer
> >>> really thought about this problem.
> >>>
> >>
> >>
> >> And reminder, commit messages should *not* be stories about how you ended
> >> up with this particular change. They should just tell "what" and "why".
> >> It seems like using mozreview leads occasionally writing stories (which is
> >> totally fine as a bugzilla comment).
> >>
> >
> > I disagree somewhat. As a reviewer, I often appreciate the extra context if
> > it helps me - the reviewer - or a future me - an archeologist or patch
> > author - understand the situation better.
> 
> That is why we have links to the bug. Bug should always be the unite of truth 
> telling
> why some change was done. Bugs tend to have so much more context about the 
> change than any individual commit message can or should have.

But then some bugs may accumulate hundreds of comments and it becomes 
unreasonable to expect future maintainers to read through them all, when the 
commit descriptions could just present a selectively-useful history of the "how 
we got to this solution".

> 
> > If that context prevents the
> > reviewer or a future patch author from asking "why didn't we do X [and
> > spending a few hours waiting for a reply or trying to find an answer]" then
> > that context was justified. I do agree there are frivolous stories that do
> > little more than entertain (e.g. the first paragraphs of
> > https://hg.mozilla.org/mozilla-central/rev/6b064f9f6e10) and we should not
> > be encouraging that style.
> >
> >
> >> Overlong unrelated commit messages just make it harder to read blame.
> >>
> >
> > This is the tail wagging the dog. Please file a bug against the tool for
> > letting best practices interfere with an important workflow.
> >

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

Re: mozIntl.DateTimeFormat

[gsquelart]

On Friday, April 7, 2017 at 11:22:17 AM UTC+12, zbran...@mozilla.com wrote:
> Hi all,
> 
> We completed the transition of the Intl handling from using OS locale, to use 
> browser UI locale.
> 
> We have a new API called mozIntl.DateTimeFormat which should be the primary 
> way we format date and time in our chrome.
> You can think of it as a regular ECMA402 DateTimeFormat on steroids.
> 
> It gives us two "shorthand" options "dateStyle" and "timeStyle" which you can 
> use instead of listing manually all options. This should lead to increased 
> consistency of our UI. On top of that, those two options allow us to tap into 
> OS regional settings to read any manual adjustments user made and respect 
> them.
> 
> Imagine that the user changed time format from hour12 to hour24. 
> mozIntl.DateTimeFormat will respect that and show the time in the current UI 
> locale, but with this adjustment.
> 
> This step is crucial for better product localizability (because now the dates 
> are in the same language as the rest of UI - think "Today is: April 5th" 
> where "April 5th" comes from date formatting and "Today is:" from l10n - we 
> want both to be in one language).
> 
> Example of how to use the new API:
> 
> ```
> 
> let dtf = mozIntl.createDateTimeFormat(undefined, {
>   dateStyle: 'long', // full | long | medium | short
>   timeStyle: 'medium // full | long | medium | short
> });
> 
> dtf.format(now);
> 
> ```
> 
> Please, use the new API for all new code and when possible, migrate old code 
> to use it.
> 
> Thanks!
> zb.

"from using OS locale, to use browser UI locale."

How is the browser UI locale set/chosen? If based on OS locale settings, great!

However, if based on (I guess) downloaded version:

Does that mean that Firefox will now ignore *my* preferred OS-wide settings? 
(e.g.: 24h clock, -MM-DD dates.)
And date/time displays would be inconsistent with the date in my OS task 
bar and most other software.

I would understand if Firefox offered a way to override that (which would be 
useful for testing IIUC), but making it the default seems disrespectful of our 
users.

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

Re: Please do NOT hand-edit web platform test MANIFEST.json files

[gsquelart]

On Saturday, March 18, 2017 at 5:01:17 AM UTC+11, Boris Zbarsky wrote:
> We have tools for this: "mach wpt-manifest-update" will do the right thing.
> 
> The typical result of hand-edits is that later changesets that do use 
> the tools end up conflicting with each other, as they all fix up the 
> incorrect hand-edits.  Please don't cause this pain for other developers 
> and the sheriffs.
> 
> Thanks,
> Boris

Oh hey, another day, another silly thought:
If this file can be generated, why is it even checked-in? :-)
(Is the ~10s extra build time unacceptable?)

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

Re: Please do NOT hand-edit web platform test MANIFEST.json files

[gsquelart]

On Tuesday, March 21, 2017 at 2:58:17 AM UTC+11, Boris Zbarsky wrote:
> On 3/19/17 12:36 AM, Nils Ohlmeier wrote:
> > Wouldn’t it make more sense to let the build system detect and reject/warn 
> > about (?) such a manual modification?
> 
> That would be ideal, but there are some issues with doing it.  We tried 
> adding a lint, but it was orange _all the time_ because the sanest 
> possible workflow of "edit a test file" or "add a test file using the 
> mach command we have for it" looked like manual modification.
> 
> We're working on a better setup for this, but in the meantime it would 
> be good if people would try to use the tools we have to make life a bit 
> less painful.
> 
> -Boris

Sorry if it's a silly suggestion:
Could the current tool insert some helpful reminders *everywhere* in the 
generated file (so it's can't be missed)?
E.g., every 2nd line would read: "// PSA: This file is auto-generated by ./mach 
wpt-manifest-update, please don't edit!" ;-)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: The future of commit access policy for core Firefox

[gsquelart]

On Sunday, March 12, 2017 at 1:23:45 AM UTC+11, smaug wrote:
> On 03/11/2017 08:23 AM, Nicholas Nethercote wrote:
> > On Sat, Mar 11, 2017 at 2:23 PM, smaug via governance <
> > gov...@lists.mozilla.org> wrote:
> >>
> >> I'd be ok to do a quick r+ if interdiff was working well.
> >
> > Depending on the relative timezones of the reviewer and reviewee, that
> > could delay landing by 24 hours or even a whole weekend.
> >
> The final r+, if it is just cosmetic changes wouldn't need to be done by the 
> same reviewer.
> 
> Perhaps we shouldn't even call the last step a review. It would be 
> "ok-to-land".
> r+ without asking any changes would implicitly contain that "ok-to-land".
> (if rebasing causes some changes, that would then need explicit "ok-to-land")
> 
> 
> 
> 
> > In general there seems to be a large amount of support in this thread for
> > continuing to allow the r+-with-minor-fixes option.
> 
> Yeah. I don't object that, but I also think that having final approval to 
> land the patch might not really be that bad
> (assuming the tools are working well enough).
> 
> >
> > Nick

If we really want to enforce a final review to prevent unwanted code to land, 
Mozilla (as a whole) will incur some costs: Reviewers spending time 
re-reviewing; delays to land (worse across tz, and which could result in the 
need to rebase again, and therefore another round of ok-to-land); and mounting 
anger at all these papercuts.

So if Mozilla is really committed to this solution, and is ready to bear the 
associated financial costs, I would suggest we recruit people who would do just 
these tasks! Could be existing sheriffs, and/or volunteering peers, and/or new 
staff with distinct job descriptions.

Hopefully they'd rubber-stamp 99% of last-minute changes, and only the more 
complex changes would be referred back to the author and reviewer.
As a bonus they could also handle trivial autoland merge issues.

In the end it should cost a similar amount of money, but would lessen the other 
costs (delays and frustration).


And while I've got the mic, a small suggestion for mozreview to help with some 
of this: We need a way for the author to add a comment explaining what was done 
between pushes (rebase, nit-fixing, other notes to reviewer, etc.); this 
comment would only appear in Bugzilla and mozreview, not in the actual patch 
commit description.
(Could be a paragraph starting with "notes-to-reviewer:", to be removed before 
autolanding.)

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

Re: All the processes

[gsquelart]

Just about the "4 good, 8 bad" part, it seems quite arbitrary -- Wouldn't that 
be hardware-dependent?
I would think users with "only" 1GB may have different needs and expectations 
from users with 16+GB.

Intuitively I don't grasp how each content process can add that much more 
memory that it would become a "major problem" jumping from 4 to 8 -- Are these 
measurements accessible somewhere, to get a sense of the magnitudes involved?

After that, of course for each machine there may be a limit we would want to 
enforce, so this discussion here is still needed.

Thanks,
Gerald

On Tuesday, March 7, 2017 at 9:13:26 AM UTC+11, Nicholas Nethercote wrote:
...
> Now for the reason I raised this: the major downside of using multiple
> processes is that it increases memory usage. Recent-ish measurements showed
> that for e10s-multi we could probably go up to 4 content processes without
> blowing it out too badly, but 8 would be a major problem.
...
> Nick
> 
> 
> 
> On Sat, Mar 4, 2017 at 11:15 AM, Nicholas Nethercote  > wrote:
> 
> > Hi,
> >
> > I want to understand all the different processes that we can and will have
> > in Firefox. Here's a list I constructed off the top of my head.
> >
> > - main process
> >
> > - content process(es): 1 on release for most users; 2 on Nightly
> >
> > - plugin process: just for Flash now?
> >
> > - gfx compositor process (bug 1264543, in Fx53)
> >
> > - file:// URL access process (bug 1147911, in Fx53)
> >
> > IIRC there was a proposal for a thumbnail generation process a while back
> > but judging by bug 1187441 that was scrapped.
> >
> > Do I have any of these details wrong? Have I missed any?
> >
> > Thanks.
> >
> > Nick
> >

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

Re: Doxygen output?

[gsquelart]

My short (<2yr) experience of the code gave me the impression that only a small 
amount of it has proper doxygen comments.
We must be frequenting different circles; or I'm somehow blind to them. :-)

Anyway, they're mainly useful when generated websites/documents are readily 
available, which it seems isn't the case (anymore). So I'm guessing people 
don't care to write/maintain them, because there is no obvious benefit at the 
moment.

I personally would welcome a push to make doxygen more official, at the very 
least for headers that get used between modules, but the more the better; and 
to have an official (or de-facto) generated website.

--
Gerald

On Monday, February 20, 2017 at 6:06:40 PM UTC+11, Henri Sivonen wrote:
> Our comments mostly try to follow the Doxygen format, and MDN says
> that the documentation team has a tool for importing Doxygen-formatted
> IDL comments into MDN articles.
> 
> Other than that, is Doxygen output from m-c input being published anywhere?
> 
> https://people-mozilla.org/~bgirard/doxygen/gfx/ is 404 these days.
> 
> -- 
> Henri Sivonen
> hsiv...@hsivonen.fi
> https://hsivonen.fi/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Should &&/|| really be at the end of lines?

[gsquelart]

On Saturday, February 18, 2017 at 10:50:24 AM UTC+11, Boris Zbarsky wrote:
> On 2/17/17 5:44 PM, gsqu...@mozilla.com wrote:
> > - People who stick with the official parser will only ever see that 
> > (pushing to mozreview will reformat everything to the official style).
> 
> Not all reviews go through mozreview.
> 
> -Boris

Not yet, if the mozreview crowd has their way. ;-)

But anyway, here's a possible solution: Instead of even modifying local files, 
your favorite editor could reformat to your favorite style on load, and 
reformat back to the official style on save.

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

Re: Should &&/|| really be at the end of lines?

[gsquelart]

On Saturday, February 18, 2017 at 9:29:06 AM UTC+11, Bobby Holley wrote:
> On Fri, Feb 17, 2017 at 2:18 PM,  wrote:
> 
> > Hi again Nick,
> >
> > Someone made me realize that I didn't fully read your message, sorry for
> > that.
> >
> > I now see that as well as &&/||, you have grepped for other operators, and
> > shown that the overwhelming usage is to put all of them at the end of lines!
> >
> > In light of this, and from what others here have discussed, I'm
> > now thinking that we probably should indeed just update our coding style
> > with whatever is used the most out there, and model our "Official
> > Formatter" on it.
> >
> >
> > Another thought, if technically possible:
> > If our main repository is going to always be under the control of some
> > official clang-format style, it should be possible for anybody to pull the
> > repository, and use a different formatter locally with their favorite
> > style. And when pushing, their modified code could be automatically
> > reformatted with the official formatter -- Everybody feels good when
> > programming! :-)
> >
> 
> Please no. That would make reviewing a nightmare.

Not sure what your concern is:
- People who stick with the official parser will only ever see that (pushing to 
mozreview will reformat everything to the official style).
- People who prefer a different style at home will have to adapt to the 
different style when reviewing others' code, or when their code gets review 
comments in a different style -- the formatter shouldn't change things so 
wildly that it would be so hard to find the corresponding location in their 
local repo.

So it's everybody's choice to avoid a nightmare, or deal with it. ;-)


> > Cheers,
> > Gerald
> >
> >
> > On Friday, February 17, 2017 at 5:16:42 PM UTC+11, Nicholas Nethercote
> > wrote:
> > > I personally have a strong preference for operators at the end of lines.
> > > The codebase seems to agree with me, judging by some rough grepping
> > ('fff'
> > > is an alias I have that's roughly equivalent to rgrep):
> > >
> > > $ fff "&&$" | wc -l
> > >   28907
> > > $ fff "^ *&&" | wc -l
> > >3751
> > >
> > > $ fff "||$" | wc -l
> > >   26429
> > > $ fff "^ *||" | wc -l
> > >2977
> > >
> > > $ fff " =$" | wc -l
> > >   39379
> > > $ fff "^ *= " | wc -l
> > > 629
> > >
> > > $ fff " +$" | wc -l
> > >   31909
> > > $ fff "^ *+$" | wc -l
> > > 491
> > >
> > > $ fff " -$" | wc -l
> > >2083
> > > $ fff "^ *-$" | wc -l
> > >  52
> > >
> > > $ fff " ==$" | wc -l
> > >   1501
> > > $ fff "^ *== " | wc -l
> > >   161
> > >
> > > $ fff " !=$" | wc -l
> > >   699
> > > $ fff "^ *!= " | wc -l
> > >   129
> > >
> > > They are rough regexps and probably have some false positives, but the
> > > numbers aren't even close; operators at the end of the line clearly
> > > dominate.
> > >
> > > I will conform for the greater good but silently weep inside every time I
> > > see it.
> > >
> > > Nick
> > >
> > > On Fri, Feb 17, 2017 at 8:47 AM,  wrote:
> > >
> > > > Question of the day:
> > > > When breaking overlong expressions, should &&/|| go at the end or the
> > > > beginning of the line?
> > > >
> > > > TL;DR: Coding style says 'end', I think we should change it to
> > > > 'beginning' for better clarity, and consistency with other operators.
> > > >
> > > >
> > > > Our coding style reads:
> > > > "Break long conditions after && and || logical connectives. See below
> > for
> > > > the rule for other operators." [1]
> > > > """
> > > > Overlong expressions not joined by && and || should break so the
> > operator
> > > > starts on the second line and starts in the same column as the start
> > of the
> > > > expression in the first line. This applies to ?:, binary arithmetic
> > > > operators including +, and member-of operators (in particular the .
> > > > operator in JavaScript, see the Rationale).
> > > >
> > > > Rationale: operator at the front of the continuation line makes for
> > faster
> > > > visual scanning, because there is no need to read to end of line. Also
> > > > there exists a context-sensitive keyword hazard in JavaScript; see bug
> > > > 442099, comment 19, which can be avoided by putting . at the start of a
> > > > continuation line in long member expression.
> > > > """ [2]
> > > >
> > > >
> > > > I initially focused on the rationale, so I thought *all* operators
> > should
> > > > go at the front of the line.
> > > >
> > > > But it seems I've been living a lie!
> > > > &&/|| should apparently be at the end, while other operators (in some
> > > > situations) should be at the beginning.
> > > >
> > > >
> > > > Now I personally think this just doesn't make sense:
> > > > - Why the distinction between &&/|| and other operators?
> > > > - Why would the excellent rationale not apply to &&/||?
> > > > - Pedantically, the style talks about 'expression *not* joined by
> > &&/||,
> > > > but what about expression that *are* joined by &&/||? (Undefined
> > 

Re: Should &&/|| really be at the end of lines?

[gsquelart]

Hi again Nick,

Someone made me realize that I didn't fully read your message, sorry for that.

I now see that as well as &&/||, you have grepped for other operators, and 
shown that the overwhelming usage is to put all of them at the end of lines!

In light of this, and from what others here have discussed, I'm now 
thinking that we probably should indeed just update our coding style with 
whatever is used the most out there, and model our "Official Formatter" on it.


Another thought, if technically possible:
If our main repository is going to always be under the control of some official 
clang-format style, it should be possible for anybody to pull the repository, 
and use a different formatter locally with their favorite style. And when 
pushing, their modified code could be automatically reformatted with the 
official formatter -- Everybody feels good when programming! :-)

Cheers,
Gerald


On Friday, February 17, 2017 at 5:16:42 PM UTC+11, Nicholas Nethercote wrote:
> I personally have a strong preference for operators at the end of lines.
> The codebase seems to agree with me, judging by some rough grepping ('fff'
> is an alias I have that's roughly equivalent to rgrep):
> 
> $ fff "&&$" | wc -l
>   28907
> $ fff "^ *&&" | wc -l
>3751
> 
> $ fff "||$" | wc -l
>   26429
> $ fff "^ *||" | wc -l
>2977
> 
> $ fff " =$" | wc -l
>   39379
> $ fff "^ *= " | wc -l
> 629
> 
> $ fff " +$" | wc -l
>   31909
> $ fff "^ *+$" | wc -l
> 491
> 
> $ fff " -$" | wc -l
>2083
> $ fff "^ *-$" | wc -l
>  52
> 
> $ fff " ==$" | wc -l
>   1501
> $ fff "^ *== " | wc -l
>   161
> 
> $ fff " !=$" | wc -l
>   699
> $ fff "^ *!= " | wc -l
>   129
> 
> They are rough regexps and probably have some false positives, but the
> numbers aren't even close; operators at the end of the line clearly
> dominate.
> 
> I will conform for the greater good but silently weep inside every time I
> see it.
> 
> Nick
> 
> On Fri, Feb 17, 2017 at 8:47 AM,  wrote:
> 
> > Question of the day:
> > When breaking overlong expressions, should &&/|| go at the end or the
> > beginning of the line?
> >
> > TL;DR: Coding style says 'end', I think we should change it to
> > 'beginning' for better clarity, and consistency with other operators.
> >
> >
> > Our coding style reads:
> > "Break long conditions after && and || logical connectives. See below for
> > the rule for other operators." [1]
> > """
> > Overlong expressions not joined by && and || should break so the operator
> > starts on the second line and starts in the same column as the start of the
> > expression in the first line. This applies to ?:, binary arithmetic
> > operators including +, and member-of operators (in particular the .
> > operator in JavaScript, see the Rationale).
> >
> > Rationale: operator at the front of the continuation line makes for faster
> > visual scanning, because there is no need to read to end of line. Also
> > there exists a context-sensitive keyword hazard in JavaScript; see bug
> > 442099, comment 19, which can be avoided by putting . at the start of a
> > continuation line in long member expression.
> > """ [2]
> >
> >
> > I initially focused on the rationale, so I thought *all* operators should
> > go at the front of the line.
> >
> > But it seems I've been living a lie!
> > &&/|| should apparently be at the end, while other operators (in some
> > situations) should be at the beginning.
> >
> >
> > Now I personally think this just doesn't make sense:
> > - Why the distinction between &&/|| and other operators?
> > - Why would the excellent rationale not apply to &&/||?
> > - Pedantically, the style talks about 'expression *not* joined by &&/||,
> > but what about expression that *are* joined by &&/||? (Undefined Behavior!)
> >
> > Based on that, I believe &&/|| should be made consistent with *all*
> > operators, and go at the beginning of lines, aligned with the first operand
> > above.
> >
> > And therefore I would propose the following changes to the coding style:
> > - Remove the lonely &&/|| sentence at [1].
> > - Rephrase the first sentence at [2] to something like: "Overlong
> > expressions should break so that the operator starts on the following line,
> > in the same column as the first operand for that operator. This applies to
> > all binary operators, including member-of operators (in particular the .
> > operator in JavaScript, see the Rationale), and extends to ?: where the 2nd
> > and third operands should be on separate lines and start in the same column
> > as the first operand."
> > - Keep the rationale at [2].
> >
> > Also, I think we should add something about where to break expressions
> > with operators of differing precedences, something like: "Overlong
> > expressions containing operators of differing precedences should first be
> > broken at the operator of lowest precedence. E.g.: 'a+b*c' should be split
> > at '+' before '*'"
> >
> >
> > A bit more context:
> > Looking at the history of the 

Re: Should &&/|| really be at the end of lines?

[gsquelart]

Of course grepping agrees with you, since it's been the coding style so far! ;-)
I was hoping to change that, if most people agreed.

While it's good to know how many people are for or against it, so that we get a 
sense of where the majority swings, I'd really like to know *why* people have 
their position. (I could learn something!)

So Nick, would you have some reasons for your "strong preference"? And what do 
you think of the opposite rationale as found in [2]?

(But I realize it's more work, so no troubles if you don't have the time to 
expand on your position here thank you for your feedback so far.)

On Friday, February 17, 2017 at 5:16:42 PM UTC+11, Nicholas Nethercote wrote:
> I personally have a strong preference for operators at the end of lines.
> The codebase seems to agree with me, judging by some rough grepping ('fff'
> is an alias I have that's roughly equivalent to rgrep):
> 
> $ fff "&&$" | wc -l
>   28907
> $ fff "^ *&&" | wc -l
>3751
> 
> $ fff "||$" | wc -l
>   26429
> $ fff "^ *||" | wc -l
>2977
> 
> $ fff " =$" | wc -l
>   39379
> $ fff "^ *= " | wc -l
> 629
> 
> $ fff " +$" | wc -l
>   31909
> $ fff "^ *+$" | wc -l
> 491
> 
> $ fff " -$" | wc -l
>2083
> $ fff "^ *-$" | wc -l
>  52
> 
> $ fff " ==$" | wc -l
>   1501
> $ fff "^ *== " | wc -l
>   161
> 
> $ fff " !=$" | wc -l
>   699
> $ fff "^ *!= " | wc -l
>   129
> 
> They are rough regexps and probably have some false positives, but the
> numbers aren't even close; operators at the end of the line clearly
> dominate.
> 
> I will conform for the greater good but silently weep inside every time I
> see it.
> 
> Nick
> 
> On Fri, Feb 17, 2017 at 8:47 AM,  wrote:
> 
> > Question of the day:
> > When breaking overlong expressions, should &&/|| go at the end or the
> > beginning of the line?
> >
> > TL;DR: Coding style says 'end', I think we should change it to
> > 'beginning' for better clarity, and consistency with other operators.
> >
> >
> > Our coding style reads:
> > "Break long conditions after && and || logical connectives. See below for
> > the rule for other operators." [1]
> > """
> > Overlong expressions not joined by && and || should break so the operator
> > starts on the second line and starts in the same column as the start of the
> > expression in the first line. This applies to ?:, binary arithmetic
> > operators including +, and member-of operators (in particular the .
> > operator in JavaScript, see the Rationale).
> >
> > Rationale: operator at the front of the continuation line makes for faster
> > visual scanning, because there is no need to read to end of line. Also
> > there exists a context-sensitive keyword hazard in JavaScript; see bug
> > 442099, comment 19, which can be avoided by putting . at the start of a
> > continuation line in long member expression.
> > """ [2]
> >
> >
> > I initially focused on the rationale, so I thought *all* operators should
> > go at the front of the line.
> >
> > But it seems I've been living a lie!
> > &&/|| should apparently be at the end, while other operators (in some
> > situations) should be at the beginning.
> >
> >
> > Now I personally think this just doesn't make sense:
> > - Why the distinction between &&/|| and other operators?
> > - Why would the excellent rationale not apply to &&/||?
> > - Pedantically, the style talks about 'expression *not* joined by &&/||,
> > but what about expression that *are* joined by &&/||? (Undefined Behavior!)
> >
> > Based on that, I believe &&/|| should be made consistent with *all*
> > operators, and go at the beginning of lines, aligned with the first operand
> > above.
> >
> > And therefore I would propose the following changes to the coding style:
> > - Remove the lonely &&/|| sentence at [1].
> > - Rephrase the first sentence at [2] to something like: "Overlong
> > expressions should break so that the operator starts on the following line,
> > in the same column as the first operand for that operator. This applies to
> > all binary operators, including member-of operators (in particular the .
> > operator in JavaScript, see the Rationale), and extends to ?: where the 2nd
> > and third operands should be on separate lines and start in the same column
> > as the first operand."
> > - Keep the rationale at [2].
> >
> > Also, I think we should add something about where to break expressions
> > with operators of differing precedences, something like: "Overlong
> > expressions containing operators of differing precedences should first be
> > broken at the operator of lowest precedence. E.g.: 'a+b*c' should be split
> > at '+' before '*'"
> >
> >
> > A bit more context:
> > Looking at the history of the coding style page, a certain "Brendan" wrote
> > that section in August 2009 [3], shortly after a discussion here [4] that
> > seemed to focus on the dot operator in Javascript. In that discussion,
> > &&/|| appear in examples at the end of lines and nobody talks about that
> > (because it 

Re: Should &&/|| really be at the end of lines?

[gsquelart]

Thank you David.
Your opinion about any *future* change to &&/|| style would be appreciated. ;-)

About "increased disruption to blame that makes understanding the history of 
our code harder":
I think some tools like http://searchfox.org/ have considerably lessened the 
impact to blame, because they make it so easy to skip over style changes.
E.g., in SearchFox, hover over the gray blame bar to the left, and click "Show 
latest version without this line", done!

On Friday, February 17, 2017 at 10:25:58 AM UTC+11, David Baron wrote:
> On Thursday 2017-02-16 15:11 -0800, Jeff Gilbert wrote:
> > I would not assume that's necessarily going to happen without
> > contention. Consistency is not a goal in and of itself.
> 
> Using clang-format on the entire tree has the massive value of:
> 
>  * reviewers not needing to point out whitespace and indentation issues
> 
>  * reduced friction for new contributors (being able to use a tool
>instead of repeatedly iterating on review comments)
> 
>  * reduced friction for existing contributors working in new areas
>of the codebase
> 
> which outweighs indvidual style preferences or historic module
> differences here.
> 
> We should try to do something as close as reasonably possible to the
> existing dominant style so that we disrupt annotate/blame the least.
> 
> 
> As for the proposal to switch to start-of-line && and || -- I
> wouldn't want to see us try to make such a change *before* we
> clang-format the entire tree, because we'll just create an
> inconsistent mess that makes it harder to figure out what the local
> style is.  I'd be slightly more willing to consider it *after* we
> clang-format the whole tree (or simultaneously, which is perhaps
> better for blame since there's only one revision to skip), but then
> we need to balance it against the cost of the increased disruption
> to blame that makes understanding the history of our code harder.
> 
> -David
> 
> > On Thu, Feb 16, 2017 at 3:04 PM,   wrote:
> > > There's an ongoing effort to use clang-format to automatically format All 
> > > The Codes [1], so I think we should try and make sure we agree (or at 
> > > least settle*) on a coding style when it's going to be enforced 
> > > everywhere. ;-)
> > >
> > > * I'm presenting my own preferences here, but in the end I'm fine with 
> > > complying with whatever we can all agree to live with. Consistence is 
> > > more important than personal preferences.
> > >
> > > g.
> > >
> > > On Friday, February 17, 2017 at 9:57:04 AM UTC+11, Jeff Gilbert wrote:
> > >> I don't visually like ||/&& at start of line, but I can't fault the
> > >> reasoning, so I'm weakly for it.
> > >> I don't think it's important enough to change existing code though.
> > >>
> > >> On Thu, Feb 16, 2017 at 1:47 PM,   wrote:
> > >> > Question of the day:
> > >> > When breaking overlong expressions, should &&/|| go at the end or the 
> > >> > beginning of the line?
> > >> >
> > >> > TL;DR: Coding style says 'end', I think we should change it to 
> > >> > 'beginning' for better clarity, and consistency with other operators.
> > >> >
> > >> >
> > >> > Our coding style reads:
> > >> > "Break long conditions after && and || logical connectives. See below 
> > >> > for the rule for other operators." [1]
> > >> > """
> > >> > Overlong expressions not joined by && and || should break so the 
> > >> > operator starts on the second line and starts in the same column as 
> > >> > the start of the expression in the first line. This applies to ?:, 
> > >> > binary arithmetic operators including +, and member-of operators (in 
> > >> > particular the . operator in JavaScript, see the Rationale).
> > >> >
> > >> > Rationale: operator at the front of the continuation line makes for 
> > >> > faster visual scanning, because there is no need to read to end of 
> > >> > line. Also there exists a context-sensitive keyword hazard in 
> > >> > JavaScript; see bug 442099, comment 19, which can be avoided by 
> > >> > putting . at the start of a continuation line in long member 
> > >> > expression.
> > >> > """ [2]
> > >> >
> > >> >
> > >> > I initially focused on the rationale, so I thought *all* operators 
> > >> > should go at the front of the line.
> > >> >
> > >> > But it seems I've been living a lie!
> > >> > &&/|| should apparently be at the end, while other operators (in some 
> > >> > situations) should be at the beginning.
> > >> >
> > >> >
> > >> > Now I personally think this just doesn't make sense:
> > >> > - Why the distinction between &&/|| and other operators?
> > >> > - Why would the excellent rationale not apply to &&/||?
> > >> > - Pedantically, the style talks about 'expression *not* joined by 
> > >> > &&/||, but what about expression that *are* joined by &&/||? 
> > >> > (Undefined Behavior!)
> > >> >
> > >> > Based on that, I believe &&/|| should be made consistent with *all* 
> > >> > operators, and go at the beginning of lines, aligned with 

Re: Should &&/|| really be at the end of lines?

[gsquelart]

Jeff, I see you're opinionated against consistency. :-)
Or you think there are other more important things?
Anyway, I'm not exactly sure what you're advocating. Could you please elaborate?

On Friday, February 17, 2017 at 10:11:25 AM UTC+11, Jeff Gilbert wrote:
> I would not assume that's necessarily going to happen without
> contention. Consistency is not a goal in and of itself.
> 
> On Thu, Feb 16, 2017 at 3:04 PM,   wrote:
> > There's an ongoing effort to use clang-format to automatically format All 
> > The Codes [1], so I think we should try and make sure we agree (or at least 
> > settle*) on a coding style when it's going to be enforced everywhere. ;-)
> >
> > * I'm presenting my own preferences here, but in the end I'm fine with 
> > complying with whatever we can all agree to live with. Consistence is more 
> > important than personal preferences.
> >
> > g.
> >
> > On Friday, February 17, 2017 at 9:57:04 AM UTC+11, Jeff Gilbert wrote:
> >> I don't visually like ||/&& at start of line, but I can't fault the
> >> reasoning, so I'm weakly for it.
> >> I don't think it's important enough to change existing code though.
> >>
> >> On Thu, Feb 16, 2017 at 1:47 PM,   wrote:
> >> > Question of the day:
> >> > When breaking overlong expressions, should &&/|| go at the end or the 
> >> > beginning of the line?
> >> >
> >> > TL;DR: Coding style says 'end', I think we should change it to 
> >> > 'beginning' for better clarity, and consistency with other operators.
> >> >
> >> >
> >> > Our coding style reads:
> >> > "Break long conditions after && and || logical connectives. See below 
> >> > for the rule for other operators." [1]
> >> > """
> >> > Overlong expressions not joined by && and || should break so the 
> >> > operator starts on the second line and starts in the same column as the 
> >> > start of the expression in the first line. This applies to ?:, binary 
> >> > arithmetic operators including +, and member-of operators (in particular 
> >> > the . operator in JavaScript, see the Rationale).
> >> >
> >> > Rationale: operator at the front of the continuation line makes for 
> >> > faster visual scanning, because there is no need to read to end of line. 
> >> > Also there exists a context-sensitive keyword hazard in JavaScript; see 
> >> > bug 442099, comment 19, which can be avoided by putting . at the start 
> >> > of a continuation line in long member expression.
> >> > """ [2]
> >> >
> >> >
> >> > I initially focused on the rationale, so I thought *all* operators 
> >> > should go at the front of the line.
> >> >
> >> > But it seems I've been living a lie!
> >> > &&/|| should apparently be at the end, while other operators (in some 
> >> > situations) should be at the beginning.
> >> >
> >> >
> >> > Now I personally think this just doesn't make sense:
> >> > - Why the distinction between &&/|| and other operators?
> >> > - Why would the excellent rationale not apply to &&/||?
> >> > - Pedantically, the style talks about 'expression *not* joined by &&/||, 
> >> > but what about expression that *are* joined by &&/||? (Undefined 
> >> > Behavior!)
> >> >
> >> > Based on that, I believe &&/|| should be made consistent with *all* 
> >> > operators, and go at the beginning of lines, aligned with the first 
> >> > operand above.
> >> >
> >> > And therefore I would propose the following changes to the coding style:
> >> > - Remove the lonely &&/|| sentence at [1].
> >> > - Rephrase the first sentence at [2] to something like: "Overlong 
> >> > expressions should break so that the operator starts on the following 
> >> > line, in the same column as the first operand for that operator. This 
> >> > applies to all binary operators, including member-of operators (in 
> >> > particular the . operator in JavaScript, see the Rationale), and extends 
> >> > to ?: where the 2nd and third operands should be on separate lines and 
> >> > start in the same column as the first operand."
> >> > - Keep the rationale at [2].
> >> >
> >> > Also, I think we should add something about where to break expressions 
> >> > with operators of differing precedences, something like: "Overlong 
> >> > expressions containing operators of differing precedences should first 
> >> > be broken at the operator of lowest precedence. E.g.: 'a+b*c' should be 
> >> > split at '+' before '*'"
> >> >
> >> >
> >> > A bit more context:
> >> > Looking at the history of the coding style page, a certain "Brendan" 
> >> > wrote that section in August 2009 [3], shortly after a discussion here 
> >> > [4] that seemed to focus on the dot operator in Javascript. In that 
> >> > discussion, &&/|| appear in examples at the end of lines and nobody 
> >> > talks about that (because it was not the main subject, and/or everybody 
> >> > agreed with it?)
> >> >
> >> > Discuss!
> >> >
> >> >
> >> > [1] 
> >> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
> >> > [2] 

Re: Should &&/|| really be at the end of lines?

[gsquelart]

There's an ongoing effort to use clang-format to automatically format All The 
Codes [1], so I think we should try and make sure we agree (or at least 
settle*) on a coding style when it's going to be enforced everywhere. ;-)

* I'm presenting my own preferences here, but in the end I'm fine with 
complying with whatever we can all agree to live with. Consistence is more 
important than personal preferences.

g.

On Friday, February 17, 2017 at 9:57:04 AM UTC+11, Jeff Gilbert wrote:
> I don't visually like ||/&& at start of line, but I can't fault the
> reasoning, so I'm weakly for it.
> I don't think it's important enough to change existing code though.
> 
> On Thu, Feb 16, 2017 at 1:47 PM,   wrote:
> > Question of the day:
> > When breaking overlong expressions, should &&/|| go at the end or the 
> > beginning of the line?
> >
> > TL;DR: Coding style says 'end', I think we should change it to 
> > 'beginning' for better clarity, and consistency with other operators.
> >
> >
> > Our coding style reads:
> > "Break long conditions after && and || logical connectives. See below for 
> > the rule for other operators." [1]
> > """
> > Overlong expressions not joined by && and || should break so the operator 
> > starts on the second line and starts in the same column as the start of the 
> > expression in the first line. This applies to ?:, binary arithmetic 
> > operators including +, and member-of operators (in particular the . 
> > operator in JavaScript, see the Rationale).
> >
> > Rationale: operator at the front of the continuation line makes for faster 
> > visual scanning, because there is no need to read to end of line. Also 
> > there exists a context-sensitive keyword hazard in JavaScript; see bug 
> > 442099, comment 19, which can be avoided by putting . at the start of a 
> > continuation line in long member expression.
> > """ [2]
> >
> >
> > I initially focused on the rationale, so I thought *all* operators should 
> > go at the front of the line.
> >
> > But it seems I've been living a lie!
> > &&/|| should apparently be at the end, while other operators (in some 
> > situations) should be at the beginning.
> >
> >
> > Now I personally think this just doesn't make sense:
> > - Why the distinction between &&/|| and other operators?
> > - Why would the excellent rationale not apply to &&/||?
> > - Pedantically, the style talks about 'expression *not* joined by &&/||, 
> > but what about expression that *are* joined by &&/||? (Undefined Behavior!)
> >
> > Based on that, I believe &&/|| should be made consistent with *all* 
> > operators, and go at the beginning of lines, aligned with the first operand 
> > above.
> >
> > And therefore I would propose the following changes to the coding style:
> > - Remove the lonely &&/|| sentence at [1].
> > - Rephrase the first sentence at [2] to something like: "Overlong 
> > expressions should break so that the operator starts on the following line, 
> > in the same column as the first operand for that operator. This applies to 
> > all binary operators, including member-of operators (in particular the . 
> > operator in JavaScript, see the Rationale), and extends to ?: where the 2nd 
> > and third operands should be on separate lines and start in the same column 
> > as the first operand."
> > - Keep the rationale at [2].
> >
> > Also, I think we should add something about where to break expressions with 
> > operators of differing precedences, something like: "Overlong expressions 
> > containing operators of differing precedences should first be broken at the 
> > operator of lowest precedence. E.g.: 'a+b*c' should be split at '+' before 
> > '*'"
> >
> >
> > A bit more context:
> > Looking at the history of the coding style page, a certain "Brendan" wrote 
> > that section in August 2009 [3], shortly after a discussion here [4] that 
> > seemed to focus on the dot operator in Javascript. In that discussion, 
> > &&/|| appear in examples at the end of lines and nobody talks about that 
> > (because it was not the main subject, and/or everybody agreed with it?)
> >
> > Discuss!
> >
> >
> > [1] 
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
> > [2] 
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
> > [3] 
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$compare?locale=en-US=7315=7314
> > [4] 
> > https://groups.google.com/d/msg/mozilla.dev.platform/Ji9lxlLCYME/zabUmQI9S-sJ
> > ___
> > dev-platform mailing list
> > dev-pl...@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform

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

Should &&/|| really be at the end of lines?

[gsquelart]

Question of the day:
When breaking overlong expressions, should &&/|| go at the end or the beginning 
of the line?

TL;DR: Coding style says 'end', I think we should change it to 
'beginning' for better clarity, and consistency with other operators.


Our coding style reads:
"Break long conditions after && and || logical connectives. See below for the 
rule for other operators." [1]
"""
Overlong expressions not joined by && and || should break so the operator 
starts on the second line and starts in the same column as the start of the 
expression in the first line. This applies to ?:, binary arithmetic operators 
including +, and member-of operators (in particular the . operator in 
JavaScript, see the Rationale).

Rationale: operator at the front of the continuation line makes for faster 
visual scanning, because there is no need to read to end of line. Also there 
exists a context-sensitive keyword hazard in JavaScript; see bug 442099, 
comment 19, which can be avoided by putting . at the start of a continuation 
line in long member expression.
""" [2]


I initially focused on the rationale, so I thought *all* operators should go at 
the front of the line.

But it seems I've been living a lie!
&&/|| should apparently be at the end, while other operators (in some 
situations) should be at the beginning.


Now I personally think this just doesn't make sense:
- Why the distinction between &&/|| and other operators?
- Why would the excellent rationale not apply to &&/||?
- Pedantically, the style talks about 'expression *not* joined by &&/||, but 
what about expression that *are* joined by &&/||? (Undefined Behavior!)

Based on that, I believe &&/|| should be made consistent with *all* operators, 
and go at the beginning of lines, aligned with the first operand above.

And therefore I would propose the following changes to the coding style:
- Remove the lonely &&/|| sentence at [1].
- Rephrase the first sentence at [2] to something like: "Overlong expressions 
should break so that the operator starts on the following line, in the same 
column as the first operand for that operator. This applies to all binary 
operators, including member-of operators (in particular the . operator in 
JavaScript, see the Rationale), and extends to ?: where the 2nd and third 
operands should be on separate lines and start in the same column as the first 
operand."
- Keep the rationale at [2].

Also, I think we should add something about where to break expressions with 
operators of differing precedences, something like: "Overlong expressions 
containing operators of differing precedences should first be broken at the 
operator of lowest precedence. E.g.: 'a+b*c' should be split at '+' before '*'"


A bit more context:
Looking at the history of the coding style page, a certain "Brendan" wrote that 
section in August 2009 [3], shortly after a discussion here [4] that seemed to 
focus on the dot operator in Javascript. In that discussion, &&/|| appear in 
examples at the end of lines and nobody talks about that (because it was not 
the main subject, and/or everybody agreed with it?)

Discuss!


[1] 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures
[2] 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators
[3] 
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style$compare?locale=en-US=7315=7314
[4] 
https://groups.google.com/d/msg/mozilla.dev.platform/Ji9lxlLCYME/zabUmQI9S-sJ
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: A reminder about MOZ_MUST_USE and [must_use]

[gsquelart]

On Friday, January 20, 2017 at 10:36:46 AM UTC+11, Bobby Holley wrote:
> On Thu, Jan 19, 2017 at 3:26 PM,  wrote:
> 
> > On Friday, January 20, 2017 at 10:13:54 AM UTC+11, Nicholas Nethercote
> > wrote:
> > > On Fri, Jan 20, 2017 at 10:01 AM,  wrote:
> > >
> > > > And the next step would be to make must-use the default, and have
> > > > MOZ_CAN_IGNORE for the rest. ;-)
> > > >
> > >
> > > I actually tried this with all XPIDL methods. After adding several
> > hundred
> > > "Unused <<" annotations for calls that legitimately didn't need to check
> > > the return value -- and I was only a fraction of the way through the
> > > codebase -- I decided that a big bang approach wasn't going to work. So I
> > > then implemented [must_use] as an incremental alternative.
> > >
> > > Nick
> >
> > I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.
> >
> 
> I think the point is that it's not obvious that "must check the return
> value" is a sufficiently-dominant common case for arbitrary return values.
> FWIW, Rust took the [must_use] rather than [can_ignore] approach too.

That's unfortunate. But real-world data must trump my idealism in the end. :-)


> It probably depends a lot on the return value type.

Makes sense.


Another idea:
Could all non-void const methods (those that don't modify *this) be 
MOZ_MUST_USE by default? They supposedly don't have side-effects, so why would 
their return value ever be ignored?


> > So in cases where a return value from a function can be ignored, instead
> > of adding "Unused <<" at all the call sites, you'd add MOZ_CAN_IGNORE in
> > front of the function declaration. Or possibly MOZ_CAN_IGNORE_TYPE if more
> > appropriate.
> >
> > Of course I'm sure it would still take a lot of work!
> >
> > Maybe there could be a slow hybrid approach:
> > - Start with Eric's suggestion to make a directory MOZ_MUST_USE'able.
> > - Add "Unused <<" for isolated failures, or add MOZ_CAN_IGNORE[_TYPE] for
> > larger failures.
> > - Gradually "infect" more directories.
> > - Once it's everywhere, make MOZ_MUST_USE the default, and remove the
> > above scaffolding.
> > - Profit!
> >
> > Gerald
> > ___
> > dev-platform mailing list
> > dev-pl...@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> >

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

Re: A reminder about MOZ_MUST_USE and [must_use]

[gsquelart]

On Friday, January 20, 2017 at 10:13:54 AM UTC+11, Nicholas Nethercote wrote:
> On Fri, Jan 20, 2017 at 10:01 AM,  wrote:
> 
> > And the next step would be to make must-use the default, and have
> > MOZ_CAN_IGNORE for the rest. ;-)
> >
> 
> I actually tried this with all XPIDL methods. After adding several hundred
> "Unused <<" annotations for calls that legitimately didn't need to check
> the return value -- and I was only a fraction of the way through the
> codebase -- I decided that a big bang approach wasn't going to work. So I
> then implemented [must_use] as an incremental alternative.
> 
> Nick

I guess my slightly-tongue-in-cheek suggestion was to reverse MOZ_MUST_USE.

So in cases where a return value from a function can be ignored, instead of 
adding "Unused <<" at all the call sites, you'd add MOZ_CAN_IGNORE in front of 
the function declaration. Or possibly MOZ_CAN_IGNORE_TYPE if more appropriate.

Of course I'm sure it would still take a lot of work!

Maybe there could be a slow hybrid approach:
- Start with Eric's suggestion to make a directory MOZ_MUST_USE'able.
- Add "Unused <<" for isolated failures, or add MOZ_CAN_IGNORE[_TYPE] for 
larger failures.
- Gradually "infect" more directories.
- Once it's everywhere, make MOZ_MUST_USE the default, and remove the above 
scaffolding.
- Profit!

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

Re: A reminder about MOZ_MUST_USE and [must_use]

[gsquelart]

And the next step would be to make must-use the default, and have 
MOZ_CAN_IGNORE for the rest. ;-)

Gerald (who is not volunteering!)

On Friday, January 20, 2017 at 9:30:13 AM UTC+11, Eric Rescorla wrote:
> What would actually be very helpful would be a way to selectively turn on
> checking of
> *all* return values in a given file/subdirectory. Is there some
> mechanism/plan for that?
> 
> Thanks,
> -Ekr
> 
> 
> On Thu, Jan 19, 2017 at 2:09 PM, Nicholas Nethercote  > wrote:
> 
> > Hi,
> >
> > We have two annotations that can be used to prevent missing return value
> > checks:
> >
> > - MOZ_MUST_USE for C++ functions where the return type indicates
> > success/failure, e.g. nsresult, bool (in some instances), and some other
> > types.
> >
> > - [must_use] for IDL methods and properties where the nsresult value should
> > be checked.
> >
> > We have *many* functions/methods/properties for which these annotations are
> > appropriate, and *many* missing return value checks. Unfortunately, trying
> > to fix these proactively is a frustrating and thankless task, because it's
> > difficult to know in advance which missing checks are likely to cause
> > problems in advance, and adding missing checks is not always
> > straightforward.
> >
> > However, if you do see clearly buggy behaviour (e.g. a crash) caused by a
> > missing return value, please take the opportunity to retroactively add the
> > annotation(s) in that case!
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1331619 is a good example of
> > such a bug, and https://bugzilla.mozilla.org/show_bug.cgi?id=1332453 is
> > the
> > follow-up to add the annotations.
> >
> > Nick
> > ___
> > dev-platform mailing list
> > dev-pl...lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> >

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

forward declarations vs includes

[gsquelart]

Controversy!

Our beloved coding style reads:
"Forward-declare classes in your header files instead of including them 
whenever possible."
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices

I guess the main goal is to keep build times lower by reducing the number of 
nested includes.

Now I've heard the view is that if a public function takes a type, the header 
should include the necessary dependent header for that type instead of just a 
forward declaration, so that users of that function will not have to include it 
themselves.
(But forward-declaring types only used internally by the header's own types is 
still fine.)

E.g., for:
  class SomeClass
  {
  public:
void foo(SomeType* aPtr);
  private:
SomeOtherType* mValue;
  }

Our coding style says that we should just have `class SomeType; class 
SomeOtherType;` above that class.

My friend would prefer `#include "SomeType.h" class SomeOtherType;`.
This way all users of SomeClass only need to include SomeClass.h, not 
SomeType.h, when they want to call SomeClass::foo.


My personal thoughts:

I can see how nice this can be for users of SomeClass.

But then, maybe not all includers of SomeClass.h would use foo (assuming 
there's more in that file).
Also some callers of SomeClass::foo may also just be passing a pointer (e.g. a 
proxy), so they don't need to include "SomeType.h" either.
And of course, this could add to the overall build time.

So I personally still prefer the coding style as is, but I wanted to discuss 
this here to see how others feel...

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

Re: GCC 4.9 now required to build on Linux/Android

[gsquelart]

On Saturday, December 24, 2016 at 3:08:21 AM UTC+11, Nathan Froyd wrote:
> Bug 1322792 has landed on inbound, which changes configure to require
> GCC 4.9 to build; our automation switched over to GCC 4.9 for our
> Linux builds earlier this week.  (Android builds have been using GCC
> 4.9 for some time.)
> 
> This change paves the way for being able to compile in C++14 mode for
> all of our Tier-1 platforms, which in turn unlocks using some C++14
> features in our codebase:
> 
> * binary literals
> * digit separators
> * generic lambdas
> * initialized lambda captures
> * return type deduction (not quite sure if we want to use this feature widely)
> [...]
> Thanks,
> -Nathan

Excellent, thank you so much!

> paves the way for being able to compile in C++14
So, can we start using the good stuff right now, or should we wait for a proper 
"go" signal?

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

Re: rvalue references: Implicit move method generation cannot be used?

[gsquelart]

Just found this:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2010/n3153.htm
Showing that implicit move can have adverse effects, especially when blindly 
applied to pre-C++11 code.

Could it be why MS doesn't want to implement it?

'=default' should theoretically be fine, since the developer explicitly shows 
that they want it -- But I got a reply by email to my OP, saying that 
'=default' actually doesn't work on our currently-used MSVC. :-(


On Sunday, May 17, 2015 at 5:38:06 PM UTC+10, gsqu...@mozilla.com wrote:
 In Using C++ in Mozilla code ( 
 https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code ) there is 
 this note:
 rvalue references: Implicit move method generation cannot be used
 First written before Aug 2013, updated by Jcranmer on January 26, 2015
 
 Could we get more details please, e.g.: Why is that (still?) true  what to 
 do about it?
 
 My guess is that some/all compilers don't know how to generate move 
 constructors  assignments, so we need to write explicit ones.
 Can we just use '=default' or is that not working either?
 
 Thanks,
 Gerald
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

rvalue references: Implicit move method generation cannot be used?

[gsquelart]

In Using C++ in Mozilla code ( 
https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code ) there is 
this note:
rvalue references: Implicit move method generation cannot be used
First written before Aug 2013, updated by Jcranmer on January 26, 2015

Could we get more details please, e.g.: Why is that (still?) true  what to do 
about it?

My guess is that some/all compilers don't know how to generate move 
constructors  assignments, so we need to write explicit ones.
Can we just use '=default' or is that not working either?

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