Status

()

defect
--
minor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

As part of the Bloom filter mfbt implementation bug bz was asking for pointers to mfbt style docs...which are a mental amalgam of a lot of comments in Bugzilla spread across a lot of bugs and are not always consistently applied, even with some care taken to be consistent from the start.  It's basically hopelessly-tribal knowledge.  We need to write this down, then we can fix everything that's inconsistent.  But one step at a time.

The /**-Javadoc-comment-for-interfaces bit is not at all what we have now, but I think that's the only real change from what's been thought to be mfbt's style rules.  /*-only was an inheritance from JS, but everyone coming from Gecko-land wants to use /**, and even JS never really had a particularly forceful argument against it, so it seems easier to go with the flow, rather than fight the tide, and mix metaphors.

The requirement that fields go at the start of the class is a weak JS engine rule (latent at best) that I don't remember mfbt deciding upon.  Newer code tends to be doing it more, because then you can have the fields first (implicitly private) and then follow with methods and such.  I've been looking at older code that doesn't have it lately, and I've been wishing JS had had it for longer.
Attachment #602253 - Flags: review?(jones.chris.g)
Oh, I can wrap the file to 80 characters if desired -- just much easier to edit initially without having to constantly think about that constraint.
Comment on attachment 602253 [details] [diff] [review]
Patch, just adds a guide, doesn't correct any existing code

>+  class Fnord
>+  {
>+      size_t s1, s2, s3, s4, s5;
>+
>+    public:
>+      Fnord(size_t s) : s1(s), s2(s), s3(s), s4(s), s5(s) { }
>+      Fnord()
>+        : s1(0), /* member initialization can be compressed if desired */
>+          s2(0),
>+          s3(0),
>+          s4(0),
>+          s5(0)

, at the start, please :)
Attachment #602253 - Flags: feedback+
(Also, no fan of the double indentation in class declarations here)
Those bits both got inherited from JS engine style, for what it's worth.
(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #4)
> for what it's worth.

Not much in my book, sorry ;)
Comment on attachment 602253 [details] [diff] [review]
Patch, just adds a guide, doesn't correct any existing code

>diff --git a/mfbt/STYLE b/mfbt/STYLE

>+== Classes and structs ==
>+
>+Inside class and structure definitions, public/private consume one level of indentation.
>+
>+  class Baz
>+  {
>+    public:

Hm, I'm not really a fan of this style.  Does js/src already require
this?

>+== Namespaces ==

>+  namespace mozilla {
>+  ...
>+  } /* namespace mozilla */
>+

Nit: "// " comment for these please.

>+== #includes ==

>+mfbt headers should be included first, alphabetically.  Standard includes should follow, separated from mfbt includes by a blank line.
>+
>+  #include "mozilla/Assertions.h"
>+  #include "mozilla/Attributes.h"
>+  
>+  #include <string.h>
>+

This has been a practical problem for me in the past.  Although specs
are supposed to make deps between system headers clear, and of course
our code isn't supposed to define macro names etc that conflict with
system headers (*cough*), I've had issues with these in the past.
What's worked most consistently for me is

 - standard C headers first
 - then standard C++ headers
 - then headers for external libs
 - then internal headers

I agree with alphabetizing within each list.

The system header dep issues I've seen in the past may have been fixed
in newer compilers (I don't even remember specific problems), but old
habits die hard ...

Fine with this as is, we can land and evolve.
Attachment #602253 - Flags: review?(jones.chris.g) → review+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> >+Inside class and structure definitions, public/private consume one level of indentation.
> >+
> >+  class Baz
> >+  {
> >+    public:
> 
> Hm, I'm not really a fan of this style.  Does js/src already require
> this?

Yeah, JS uses this, although it couches it in terms of "half-indent" for the public/private labels (JS using four-space indentation).

> This has been a practical problem for me in the past.  Although specs
> are supposed to make deps between system headers clear, and of course
> our code isn't supposed to define macro names etc that conflict with
> system headers (*cough*), I've had issues with these in the past.
> What's worked most consistently for me is
> 
>  - standard C headers first
>  - then standard C++ headers
>  - then headers for external libs
>  - then internal headers

I would be cool with this, but for StandardInteger.h (the new name for StdInt.h, after include-path concerns mandated a change).  Specifically, I'm a little worried about our overrides not playing nicely with standard headers (well, POSIX-y or platform headers, not necessarily libc headers) that have already included <stdint.h> somehow.  I guess neither ordering is really all that nice or believable for that issue.  :-\  But...

> Fine with this as is, we can land and evolve.

Definitely.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c49e75123d62
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/c49e75123d62
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
> (Also, no fan of the double indentation in class declarations here)

Me either.  While it's true that both SM and MFBT use 2-space indents for private/protected/public and 4-space indents for everything else in a class, in SM that represents half and single indents, whereas in MFBT that represent single and double indents, which is a total botch.
You need to log in before you can comment on or make changes to this bug.