Chromium Code Reviews| OLD | NEW |
|---|---|
| (Empty) | |
| 1 Coding Style Guidelines | |
| 2 ======================= | |
| 3 | |
| 4 These conventions have evolved over time. Some of the earlier code in both | |
| 5 projects doesn’t strictly adhere to the guidelines. However, as the code evolves | |
| 6 we hope to make the existing code conform to the guildelines. | |
| 7 | |
| 8 Files | |
| 9 ----- | |
| 10 | |
| 11 We use .cpp and .h as extensions for c++ source and header files. We use | |
| 12 foo_impl.h for headers with inline definitions for class foo. | |
| 13 | |
| 14 Headers that aren’t meant for public consumption should be placed in src | |
| 15 directories so that they aren’t in a client’s search path. | |
| 16 | |
| 17 We prefer to minimize includes. If forward declaring a name in a header is | |
| 18 sufficient then that is preferred to an include. | |
| 19 | |
| 20 Forward declarations and file includes should be in alphabetical order (but we | |
| 21 aren't very strict about it). | |
| 22 | |
| 23 Do not use #if/#ifdef before including "SkTypes.h" (directly or indirectly). | |
| 24 | |
| 25 We use spaces not tabs (4 of them). | |
| 26 | |
| 27 We use Unix style endlines (LF). | |
| 28 | |
| 29 We prefer no trailing whitespace but aren't very strict about it. | |
| 30 | |
| 31 We wrap lines at 100 columns unless it is excessively ugly (use your judgement). | |
| 32 The soft line length limit was changed from 80 to 100 columns in June 2012. Thus , | |
| 33 most files still adhere to the 80 column limit. It is not necessary or worth | |
| 34 significant effort to promote 80 column wrapped files to 100 columns. Please | |
| 35 don't willy-nilly insert longer lines in 80 column wrapped files. Either be | |
| 36 consistent with the surrounding code or, if you really feel the need, promote | |
| 37 the surrounding code to 100 column wrapping. | |
| 38 | |
| 39 Naming | |
| 40 ------ | |
| 41 | |
| 42 Both projects use a prefix to designate that they are Skia prefix for classes, | |
| 43 enums, structs, typedefs etc is Sk. Ganesh’s is Gr. Nested types should not be | |
| 44 prefixed. | |
| 45 | |
| 46 ~~~~ | |
|
jcgregorio
2015/01/21 17:55:39
<!--?prettify?-->
here and below
| |
| 47 class SkClass { | |
| 48 public: | |
| 49 class HelperClass { | |
| 50 ... | |
| 51 }; | |
| 52 }; | |
| 53 ~~~~ | |
| 54 | |
| 55 Data fields in structs, classes, unions begin with lowercase f and are then | |
| 56 camel capped. | |
| 57 | |
| 58 ~~~~ | |
| 59 struct GrCar { | |
| 60 ... | |
| 61 float fMilesDriven; | |
| 62 ... | |
| 63 }; | |
| 64 ~~~~ | |
| 65 | |
| 66 Globals variables are similar but prefixed with g and camel-capped | |
| 67 | |
| 68 ~~~~ | |
| 69 bool gLoggingEnabled | |
| 70 Local variables begin lowercases and are camel-capped. | |
| 71 | |
| 72 int herdCats(const Array& cats) { | |
| 73 int numCats = cats.count(); | |
| 74 } | |
| 75 ~~~~ | |
| 76 | |
| 77 Enum values are prefixed with k and have post fix that consists of an underscore | |
| 78 and singular name of the enum name. The enum itself should be singular for | |
| 79 exclusive values or plural for a bitfield. If a count is needed it is | |
| 80 k<singular enum name>Count and not be a member of the enum (see example): | |
| 81 | |
| 82 ~~~~ | |
| 83 enum SkPancakeType { | |
| 84 kBlueberry_PancakeType, | |
| 85 kPlain_PancakeType, | |
| 86 kChocolateChip_PancakeType, | |
| 87 | |
| 88 kLast_PancakeType = kChocolateChip_PancakeType | |
| 89 }; | |
| 90 | |
| 91 static const SkPancakeType kPancakeTypeCount = kLast_PancakeType + 1; | |
| 92 ~~~~ | |
| 93 | |
| 94 A bitfield: | |
| 95 | |
| 96 ~~~~ | |
| 97 enum SkSausageIngredientBits { | |
| 98 kFennel_SuasageIngredientBit = 0x1, | |
| 99 kBeef_SausageIngredientBit = 0x2 | |
| 100 }; | |
| 101 ~~~~ | |
| 102 | |
| 103 or: | |
| 104 | |
| 105 ~~~~ | |
| 106 enum SkMatrixFlags { | |
| 107 kTranslate_MatrixFlag = 0x1, | |
| 108 kRotate_MatrixFlag = 0x2 | |
| 109 }; | |
| 110 ~~~~ | |
| 111 | |
| 112 Exception: anonymous enums can be used to declare integral constants, e.g.: | |
| 113 | |
| 114 ~~~~ | |
| 115 enum { kFavoriteNumber = 7 }; | |
| 116 ~~~~ | |
| 117 | |
| 118 Macros are all caps with underscores between words. Macros that have greater | |
| 119 than file scope should be prefixed SK or GR. | |
| 120 | |
| 121 Static non-class functions in implementation files are lower case with | |
| 122 underscores separating words: | |
| 123 | |
| 124 ~~~~ | |
| 125 static inline bool tastes_like_chicken(Food food) { | |
| 126 return kIceCream_Food != food; | |
| 127 } | |
| 128 ~~~~ | |
| 129 | |
| 130 Externed functions or static class functions are camel-capped with an initial ca p: | |
| 131 | |
| 132 ~~~~ | |
| 133 bool SkIsOdd(int n); | |
| 134 | |
| 135 class SkFoo { | |
| 136 public: | |
| 137 static int FooInstanceCount(); | |
| 138 }; | |
| 139 ~~~~ | |
| 140 | |
| 141 Macros | |
| 142 ------ | |
| 143 | |
| 144 Ganesh macros that are GL-specific should be prefixed GR_GL. | |
| 145 | |
| 146 ~~~~ | |
| 147 #define GR_GL_TEXTURE0 0xdeadbeef | |
| 148 ~~~~ | |
| 149 | |
| 150 Ganesh prefers that macros are always defined and the use of #if MACRO rather th an | |
| 151 #ifdef MACRO. | |
| 152 | |
| 153 ~~~~ | |
| 154 #define GR_GO_SLOWER 0 | |
| 155 ... | |
| 156 #if GR_GO_SLOWER | |
| 157 Sleep(1000); | |
| 158 #endif | |
| 159 ~~~~ | |
| 160 | |
| 161 Skia tends to use #ifdef SK_MACRO for boolean flags. | |
| 162 | |
| 163 Braces | |
| 164 ------ | |
| 165 | |
| 166 Open braces don’t get a newline. “else” and “else if” appear on same line as | |
| 167 opening and closing braces unless preprocessor conditional compilation | |
| 168 interferes. Braces are always used with if, else, while, for, and do. | |
| 169 | |
| 170 ~~~~ | |
| 171 if (...) { | |
| 172 oneOrManyLines; | |
| 173 } | |
| 174 | |
| 175 if (...) { | |
| 176 oneOrManyLines; | |
| 177 } else if (...) { | |
| 178 oneOrManyLines; | |
| 179 } else { | |
| 180 oneOrManyLines; | |
| 181 } | |
| 182 | |
| 183 for (...) { | |
| 184 oneOrManyLines; | |
| 185 } | |
| 186 | |
| 187 while (...) { | |
| 188 oneOrManyLines; | |
| 189 } | |
| 190 | |
| 191 void function(...) { | |
| 192 oneOrManyLines; | |
| 193 } | |
| 194 | |
| 195 if (!error) { | |
| 196 proceed_as_usual(); | |
| 197 } | |
| 198 #if HANDLE_ERROR | |
| 199 else { | |
| 200 freak_out(); | |
| 201 } | |
| 202 #endif | |
| 203 ~~~~ | |
| 204 | |
| 205 Flow Control | |
| 206 ------------ | |
| 207 | |
| 208 There is a space between flow control words and parentheses and between | |
| 209 parentheses and braces: | |
| 210 | |
| 211 ~~~~ | |
| 212 while (...) { | |
| 213 } | |
| 214 | |
| 215 do { | |
| 216 } while(...); | |
| 217 | |
| 218 switch (...) { | |
| 219 ... | |
| 220 } | |
| 221 ~~~~ | |
| 222 | |
| 223 Cases and default in switch statements are indented from the switch. | |
| 224 | |
| 225 ~~~~ | |
| 226 switch (color) { | |
| 227 case kBlue: | |
| 228 ... | |
| 229 break; | |
| 230 case kGreen: | |
| 231 ... | |
| 232 break; | |
| 233 ... | |
| 234 default: | |
| 235 ... | |
| 236 break; | |
| 237 } | |
| 238 ~~~~ | |
| 239 | |
| 240 Fallthrough from one case to the next is commented unless it is trivial: | |
| 241 | |
| 242 ~~~~ | |
| 243 switch (recipe) { | |
| 244 ... | |
| 245 case kCheeseOmelette_Recipe: | |
| 246 ingredients |= kCheese_Ingredient; | |
| 247 // fallthrough | |
| 248 case kPlainOmelette_Recipe: | |
| 249 ingredients |= (kEgg_Ingredient | kMilk_Ingredient); | |
| 250 break; | |
| 251 ... | |
| 252 } | |
| 253 ~~~~ | |
| 254 | |
| 255 When a block is needed to declare variables within a case follow this pattern: | |
| 256 | |
| 257 ~~~~ | |
| 258 switch (filter) { | |
| 259 ... | |
| 260 case kGaussian_Filter: { | |
| 261 Bitmap srcCopy = src->makeCopy(); | |
| 262 ... | |
| 263 break; | |
| 264 } | |
| 265 ... | |
| 266 }; | |
| 267 ~~~~ | |
| 268 | |
| 269 Classes | |
| 270 ------- | |
| 271 | |
| 272 Unless there is a need for forward declaring something, class declarations | |
| 273 should be ordered public, protected, private. Each should be preceded by a | |
| 274 newline. Within each visibility section (public, private), fields should not be | |
| 275 intermixed with methods. | |
| 276 | |
| 277 ~~~~ | |
| 278 class SkFoo { | |
| 279 | |
| 280 public: | |
| 281 ... | |
| 282 | |
| 283 protected: | |
| 284 ... | |
| 285 | |
| 286 private: | |
| 287 SkBar fBar; | |
| 288 ... | |
| 289 | |
| 290 void barHelper(...); | |
| 291 ... | |
| 292 }; | |
| 293 ~~~~ | |
| 294 | |
| 295 Subclasses should have a private typedef of their super class called INHERITED: | |
| 296 | |
| 297 ~~~~ | |
| 298 class GrDillPickle : public GrPickle { | |
| 299 ... | |
| 300 private: | |
| 301 typedef GrPickle INHERITED; | |
| 302 }; | |
| 303 ~~~~ | |
| 304 | |
| 305 Virtual functions that are overridden in derived classes should use SK_OVERRIDE | |
| 306 (and not the override keyword). The virtual keyword can be omitted. | |
| 307 | |
| 308 ~~~~ | |
| 309 void myVirtual() SK_OVERRIDE { | |
| 310 } | |
| 311 ~~~~ | |
| 312 | |
| 313 This should be the last element of their private section, and all references to | |
| 314 base-class implementations of a virtual function should be explicitly qualified: | |
| 315 | |
| 316 ~~~~ | |
| 317 void myVirtual() SK_OVERRIDE { | |
| 318 ... | |
| 319 this->INHERITED::myVirtual(); | |
| 320 ... | |
| 321 } | |
| 322 ~~~~ | |
| 323 | |
| 324 As in the above example, derived classes that redefine virtual functions should | |
| 325 use SK_OVERRIDE to note that explicitly. | |
| 326 | |
| 327 Constructor initializers should be one per line, indented, with punctuation | |
| 328 placed before the initializer. This is a fairly new rule so much of the existing | |
| 329 code is non-conforming. Please fix as you go! | |
| 330 | |
| 331 ~~~~ | |
| 332 GrDillPickle::GrDillPickle() | |
| 333 : GrPickle() | |
| 334 , fSize(kDefaultPickleSize) { | |
| 335 ... | |
| 336 } | |
| 337 ~~~~ | |
| 338 | |
| 339 Constructors that take one argument should almost always be explicit, with | |
| 340 exceptions made only for the (rare) automatic compatibility class. | |
| 341 | |
| 342 ~~~~ | |
| 343 class Foo { | |
| 344 explicit Foo(int x); // Good. | |
| 345 Foo(float y); // Spooky implicit conversion from float to Foo. No n o no! | |
| 346 ... | |
| 347 }; | |
| 348 ~~~~ | |
| 349 | |
| 350 Method calls within method calls should be prefixed with dereference of the | |
| 351 'this' pointer. For example: | |
| 352 | |
| 353 ~~~~ | |
| 354 this->method(); | |
| 355 Memory Managemt | |
| 356 ~~~~ | |
| 357 | |
| 358 All memory allocation should be routed through SkNEW and its variants. These are | |
| 359 #defined in SkPostConfig.h, but the correct way to get access to the config | |
| 360 system is to #include SkTypes.h, which will allow external users of the library | |
| 361 to provide a custom memory manager or other adaptations. | |
| 362 | |
| 363 ~~~~ | |
| 364 SkNEW(type_name) | |
| 365 SkNEW_ARGS(type_name, args) | |
| 366 SkNEW_ARRAY(type_name, count) | |
| 367 SkNEW_PLACEMENT(buf, type_name) | |
| 368 SkNEW_PLACEMENT_ARGS(buf, type_name, args) | |
| 369 SkDELETE(obj) | |
| 370 SkDELETE_ARRAY(array) | |
| 371 ~~~~ | |
| 372 | |
| 373 Comparisons | |
| 374 ----------- | |
| 375 | |
| 376 We prefer that equality operators between lvalues and rvalues place the lvalue | |
| 377 on the right: | |
| 378 | |
| 379 ~~~~ | |
| 380 if (7 == luckyNumber) { | |
| 381 ... | |
| 382 } | |
| 383 ~~~~ | |
| 384 | |
| 385 However, inequality operators need not follow this rule: | |
| 386 | |
| 387 ~~~~ | |
| 388 if (count > 0) { | |
| 389 ... | |
| 390 } | |
| 391 ~~~~ | |
| 392 | |
| 393 Comments | |
| 394 | |
| 395 We use doxygen-style comments. | |
| 396 | |
| 397 For grouping or separators in an implementation file we use 80 slashes | |
| 398 | |
| 399 ~~~~ | |
| 400 void SkClassA::foo() { | |
| 401 ... | |
| 402 } | |
| 403 | |
| 404 //////////////////////////////////////////////////////////////// | |
| 405 | |
| 406 void SkClassB::bar() { | |
| 407 ... | |
| 408 } | |
| 409 ~~~~ | |
| 410 | |
| 411 Integer Types | |
| 412 ------------- | |
| 413 | |
| 414 We follow the Google C++ guide for ints and are slowly making older code conform to this | |
| 415 | |
| 416 (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types) | |
| 417 | |
| 418 Summary: Use int unless you have need a guarantee on the bit count, then use | |
| 419 stdint.h types (int32_t, etc). Assert that counts, etc are not negative instead | |
| 420 of using unsigned. Bitfields use uint32_t unless they have to be made shorter | |
| 421 for packing or performance reasons. | |
| 422 | |
| 423 NULL, 0 | |
| 424 ------- | |
| 425 | |
| 426 Use NULL for pointers, 0 for ints. We prefer explicit NULL comparisons when | |
| 427 checking for NULL pointers (as documentation): | |
| 428 | |
| 429 ~~~~ | |
| 430 if (NULL == x) { // slightly preferred over if (x) | |
| 431 ... | |
| 432 } | |
| 433 ~~~~ | |
| 434 | |
| 435 When checking non-NULL pointers explicit comparisons are not required because it | |
| 436 reads like a double negative: | |
| 437 | |
| 438 ~~~~ | |
| 439 if (x) { // slightly preferred over if (NULL != x) | |
| 440 ... | |
| 441 } | |
| 442 ~~~~ | |
| 443 | |
| 444 Returning structs | |
| 445 ----------------- | |
| 446 | |
| 447 If the desired behavior is for a function to return a struct, we prefer using a | |
| 448 struct as an output parameter | |
| 449 | |
| 450 ~~~~ | |
| 451 void modify_foo(SkFoo* foo) { | |
| 452 // Modify foo | |
| 453 } | |
| 454 ~~~~ | |
| 455 | |
| 456 Then the function can be called as followed: | |
| 457 | |
| 458 ~~~~ | |
| 459 SkFoo foo; | |
| 460 modify_foo(&foo); | |
| 461 ~~~~ | |
| 462 | |
| 463 This way, if return value optimization cannot be used there is no performance | |
| 464 hit. It also means that modify_foo can actually return a boolean for whether the | |
| 465 call was successful. In this case, initialization of foo can potentially be | |
| 466 skipped on failure (assuming the constructor for SkFoo does no initialization). | |
| 467 | |
| 468 ~~~~ | |
| 469 bool modify_foo(SkFoo* foo) { | |
| 470 if (some_condition) { | |
| 471 // Modify foo | |
| 472 return true; | |
| 473 } | |
| 474 // Leave foo unmodified | |
| 475 return false; | |
| 476 } | |
| 477 ~~~~ | |
| 478 | |
| 479 Function Parameters | |
| 480 ------------------- | |
| 481 | |
| 482 Mandatory constant object parameters are passed to functions as const references | |
| 483 if they are not retained by the receiving function. Optional constant object | |
| 484 parameters are passed to functions as const pointers. Objects that the called | |
| 485 function will retain, either directly or indirectly, are passed as pointers. | |
| 486 Variable (i.e. mutable) object parameters are passed to functions as pointers. | |
| 487 | |
| 488 ~~~~ | |
| 489 // src and paint are optional | |
| 490 void SkCanvas::drawBitmapRect(const SkBitmap& bitmap, const SkIRect* src, | |
| 491 const SkRect& dst, const SkPaint* paint = NULL); | |
| 492 // metrics is mutable (it is changed by the method) | |
| 493 SkScalar SkPaint::getFontMetrics(FontMetric* metrics, SkScalar scale) const; | |
| 494 // A reference to foo is retained by SkContainer | |
| 495 void SkContainer::insert(const SkFoo* foo); | |
| 496 ~~~~ | |
| 497 | |
| 498 If function arguments or parameters do not all fit on one line, they may be | |
| 499 lined up with the first parameter on the same line | |
| 500 | |
| 501 ~~~~ | |
| 502 void drawBitmapRect(const SkBitmap& bitmap, const SkRect& dst, | |
| 503 const SkPaint* paint = NULL) { | |
| 504 this->drawBitmapRectToRect(bitmap, NULL, dst, paint, | |
| 505 kNone_DrawBitmapRectFlag); | |
| 506 } | |
| 507 ~~~~ | |
| 508 | |
| 509 or placed on the next line indented eight spaces | |
| 510 | |
| 511 ~~~~ | |
| 512 void drawBitmapRect( | |
| 513 const SkBitmap& bitmap, const SkRect& dst, | |
| 514 const SkPaint* paint = NULL) { | |
| 515 this->drawBitmapRectToRect( | |
| 516 bitmap, NULL, dst, paint, kNone_DrawBitmapRectFlag); | |
| 517 } | |
| 518 ~~~~ | |
| 519 | |
| 520 Python | |
| 521 ------ | |
| 522 | |
| 523 Python code follows the [Google Python Style Guide](http://google-styleguide.goo glecode.com/svn/trunk/pyguide.html). | |
| 524 | |
| OLD | NEW |