minor style guide updates and editing

Change-Id: I80b1f810b284b686d80639d7a216a32589336801
Reviewed-on: https://skia-review.googlesource.com/114025
Reviewed-by: Brian Salomon <bsalomon@google.com>

1 files changed, 37 insertions, 97 deletions

diff --git a/site/dev/contrib/style.md b/site/dev/contrib/style.md
index bded3a3b70..9edf83dcdb 100644
--- a/site/dev/contrib/style.md
+++ b/site/dev/contrib/style.md
@@ -8,11 +8,11 @@ we hope to make the existing code conform to the guildelines.
 Files
 -----
 
-We use .cpp and .h as extensions for c++ source and header files. We use
-foo_impl.h for headers with inline definitions for class foo.
+We use .cpp and .h as extensions for c++ source and header files.
 
 Headers that aren't meant for public consumption should be placed in src
-directories so that they aren't in a client's search path.
+directories so that they aren't in a client's search path, or in
+include/private if they need to be used by public headers.
 
 We prefer to minimize includes. If forward declaring a name in a header is
 sufficient then that is preferred to an include.
@@ -22,8 +22,9 @@ aren't very strict about it).
 
 <span id="no-define-before-sktypes"></span>
 Do not use #if/#ifdef before including "SkTypes.h" (directly or indirectly).
+Most things you'd #if on tend to not yet be decided until SkTypes.h.
 
-We use spaces not tabs (4 of them).
+We use 4 spaces, not tabs.
 
 We use Unix style endlines (LF).
 
@@ -54,7 +55,7 @@ public:
 };
 ~~~~
 
-Data fields in structs, classes, unions begin with lowercase f and are then 
+Data fields in structs, classes, unions begin with lowercase f and are then
 camel capped.
 
 <!--?prettify?-->
@@ -81,7 +82,8 @@ int herdCats(const Array& cats) {
 Enum values are prefixed with k. Unscoped enum values are post fixed with
 an underscore and singular name of the enum name. The enum itself should be
 singular for exclusive values or plural for a bitfield. If a count is needed it
-is  `k<singular enum name>Count` and not be a member of the enum (see example):
+is  `k<singular enum name>Count` and not be a member of the enum (see example),
+or a kLast member of the enum is fine too.
 
 <!--?prettify?-->
 ~~~~
@@ -98,7 +100,7 @@ enum SkPancakeType {
      kBlueberry_PancakeType,
      kPlain_PancakeType,
      kChocolateChip_PancakeType,
-    
+
      kLast_PancakeType = kChocolateChip_PancakeType
 };
 
@@ -167,7 +169,7 @@ Ganesh macros that are GL-specific should be prefixed GR_GL.
 #define GR_GL_TEXTURE0 0xdeadbeef
 ~~~~
 
-Ganesh prefers that macros are always defined and the use of `#if MACRO` rather than 
+Ganesh prefers that macros are always defined and the use of `#if MACRO` rather than
 `#ifdef MACRO`.
 
 <!--?prettify?-->
@@ -227,7 +229,7 @@ else {
 Flow Control
 ------------
 
-There is a space between flow control words and parentheses and between 
+There is a space between flow control words and parentheses and between
 parentheses and braces:
 
 <!--?prettify?-->
@@ -252,7 +254,7 @@ switch (color) {
         ...
         break;
     case kGreen:
-        ... 
+        ...
         break;
     ...
     default:
@@ -284,7 +286,7 @@ When a block is needed to declare variables within a case follow this pattern:
 switch (filter) {
     ...
     case kGaussian_Filter: {
-        Bitmap srcCopy = src->makeCopy(); 
+        Bitmap srcCopy = src->makeCopy();
         ...
         break;
     }
@@ -298,7 +300,7 @@ Classes
 Unless there is a need for forward declaring something, class declarations
 should be ordered `public`, `protected`, `private`. Each should be preceded by a
 newline. Within each visibility section (`public`, `private`), fields should not be
-intermixed with methods.
+intermixed with methods.  It's nice to keep all data fields together at the end.
 
 <!--?prettify?-->
 ~~~~
@@ -308,13 +310,13 @@ public:
     ...
 
 protected:
-    ...        
+    ...
 
 private:
-    SkBar fBar;
+    void barHelper(...);
     ...
 
-    void barHelper(...);
+    SkBar fBar;
     ...
 };
 ~~~~
@@ -330,8 +332,8 @@ private:
 };
 ~~~~
 
-Virtual functions that are overridden in derived classes should use override
-(and not the override keyword). The virtual keyword can be omitted.
+Virtual functions that are overridden in derived classes should use override,
+and the virtual keyword should be omitted.
 
 <!--?prettify?-->
 ~~~~
@@ -339,8 +341,8 @@ void myVirtual() override {
 }
 ~~~~
 
-This should be the last element of their private section, and all references to 
-base-class implementations of a virtual function should be explicitly qualified:
+All references to base-class implementations of a virtual function
+should be explicitly qualified:
 
 <!--?prettify?-->
 ~~~~
@@ -351,9 +353,6 @@ void myVirtual() override {
 }
 ~~~~
 
-As in the above example, derived classes that redefine virtual functions should
-use override to note that explicitly.
-
 Constructor initializers should be one per line, indented, with punctuation
 placed before the initializer. This is a fairly new rule so much of the existing
 code is non-conforming. Please fix as you go!
@@ -367,7 +366,7 @@ GrDillPickle::GrDillPickle()
 }
 ~~~~
 
-Constructors that take one argument should almost always be explicit, with 
+Constructors that take one argument should almost always be explicit, with
 exceptions made only for the (rare) automatic compatibility class.
 
 <!--?prettify?-->
@@ -379,7 +378,7 @@ class Foo {
 };
 ~~~~
 
-Method calls within method calls should be prefixed with dereference of the 
+Method calls within method calls should be prefixed with dereference of the
 'this' pointer. For example:
 
 <!--?prettify?-->
@@ -387,26 +386,6 @@ Method calls within method calls should be prefixed with dereference of the
 this->method();
 ~~~~
 
-Comments
---------
-
-We use doxygen-style comments.
-
-For grouping or separators in an implementation file we use 80 slashes
-
-<!--?prettify?-->
-~~~~
-void SkClassA::foo() {
-    ...
-}
-
-////////////////////////////////////////////////////////////////
-
-void SkClassB::bar() {
-    ...
-}
-~~~~
-
 Integer Types
 -------------
 
@@ -420,10 +399,10 @@ of using unsigned. Bitfields use `uint32_t` unless they have to be made shorter
 for packing or performance reasons.
 
 `nullptr`, 0
--------
+------------
 
-Use `nullptr` for pointers, 0 for ints. We prefer explicit `nullptr` comparisons when
-checking for `nullptr` pointers (as documentation):
+Use `nullptr` for pointers, 0 for ints. We suggest explicit `nullptr` comparisons when
+checking for `nullptr` pointers, as documentation:
 
 <!--?prettify?-->
 ~~~~
@@ -432,8 +411,8 @@ if (nullptr == x) {  // slightly preferred over if (!x)
 }
 ~~~~
 
-When checking non-`nullptr` pointers explicit comparisons are not required because it
-reads like a double negative:
+When checking non-`nullptr` pointers we think implicit comparisons read better than
+an explicit comparison's double negative:
 
 <!--?prettify?-->
 ~~~~
@@ -442,65 +421,26 @@ if (x) {  // slightly preferred over if (nullptr != x)
 }
 ~~~~
 
-Returning structs
------------------
-
-If the desired behavior is for a function to return a struct, we prefer using a
-struct as an output parameter
-
-<!--?prettify?-->
-~~~~
-void modify_foo(SkFoo* foo) {
-    // Modify foo
-}
-~~~~
-
-Then the function can be called as followed:
-
-<!--?prettify?-->
-~~~~
-SkFoo foo;
-modify_foo(&foo);
-~~~~
-
-This way, if return value optimization cannot be used there is no performance
-hit. It also means that `modify_foo` can actually return a boolean for whether the
-call was successful. In this case, initialization of `foo` can potentially be
-skipped on failure (assuming the constructor for SkFoo does no initialization).
-
-<!--?prettify?-->
-~~~~
-bool modify_foo(SkFoo* foo) {
-    if (some_condition) {
-        // Modify foo
-        return true;
-    }
-    // Leave foo unmodified
-    return false;
-}
-~~~~
-
 Function Parameters
 -------------------
 
-Mandatory constant object parameters are passed to functions as `const` references
-if they are not retained by the receiving function. Optional constant object
-parameters are passed to functions as const pointers. Objects that the called
-function will retain, either directly or indirectly, are passed as pointers.
-Variable (i.e. mutable) object parameters are passed to functions as pointers.
+Mandatory constant object parameters are passed to functions as const references.
+Optional constant object parameters are passed to functions as const pointers.
+Mutable object parameters are passed to functions as pointers.
+We very rarely pass anything by non-const reference.
 
 <!--?prettify?-->
 ~~~~
 // src and paint are optional
-void SkCanvas::drawBitmapRect(const SkBitmap& bitmap, const SkIRect* src, 
-                             const SkRect& dst, const SkPaint* paint = nullptr);
+void SkCanvas::drawBitmapRect(const SkBitmap& bitmap, const SkIRect* src,
+                              const SkRect& dst, const SkPaint* paint = nullptr);
+
 // metrics is mutable (it is changed by the method)
 SkScalar SkPaint::getFontMetrics(FontMetric* metrics, SkScalar scale) const;
-// A reference to foo is retained by SkContainer
-void SkContainer::insert(const SkFoo* foo);
+
 ~~~~
 
-If function arguments or parameters do not all fit on one line, the overflowing 
+If function arguments or parameters do not all fit on one line, the overflowing
 parameters may be lined up with the first parameter on the next line
 
 <!--?prettify?-->