-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more rules based on my thinking. #1
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,12 @@ See [C++ Programmer's Toolbox](#c-programmers-toolbox) for details. | |
2. **Forward Declarations** | ||
|
||
See [Google Style Guide](https://google.github.io/styleguide/cppguide.html#Forward_Declarations). | ||
|
||
An exception for this rule is: | ||
|
||
* Use a Private Data Class for exposed API, especially for shared library. | ||
* look more details in [Expose API/Shared Library]. | ||
|
||
3. **Inline Functions** | ||
|
||
**Rule of thumb**: do not inline a function if it is more than 10 | ||
|
@@ -85,10 +91,11 @@ See [C++ Programmer's Toolbox](#c-programmers-toolbox) for details. | |
## Namespaces | ||
|
||
1. **Unnamed Namespace** | ||
* Use them in `.cpp` file to hide functions or variables you do | ||
not want expose. | ||
* **Do not** use them in `.h` files. | ||
* Sometimes you may want to expose internal functions or | ||
* ** Do not** Use them in `.cpp`/`.h` files. Using `static` for hidding | ||
symbols in source files. The anonymous namespace will generate a namespace | ||
when compiling, and could generate a different namespace name each time. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/compiling/compiled/ |
||
It will breaks the ABIs, make dubug harder. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/dubug/debug/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这并不会影响使用我们的shard libraries的应用程序才对吧?因为unnamed namespace里包括的都是应用程序不会调用的符号。 另外,这里的讨论和“break ABI”没有关系吧。所谓break是说如果我们把一个symbol从某个namespace挪到了另一个namespace里,那当然会break ABI,因为这实际上break了API——因为这是给symbol重新起了一个名字了。 |
||
* Sometimes you may want to expose internal functions or | ||
variables just for testing purpose. In this case it is better | ||
to declare them in | ||
[internal-only namespaces](cases/internal_only_namespaces.md). | ||
|
@@ -129,6 +136,8 @@ See [C++ Programmer's Toolbox](#c-programmers-toolbox) for details. | |
1. **Copy Constructor and Move Constructor** | ||
* If you provide copy constructor and/or move constructor, | ||
provide the corresponding `operator=` overload as well. | ||
* Class should be disable copy/move by default. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this statement is too strong. I think for a style guide/primer it is much more valuable if we could explain trade-offs in detail, like what you did for unnamed namespace :) Copy and move constructors can be very useful, and the programmer should use their best judgement when implementing/disabling them. |
||
|
||
2. **Inheritance** | ||
* All inheritance should be **public**. If you want to do | ||
private inheritance, you should be including an instance of | ||
|
@@ -161,14 +170,17 @@ See [C++ Programmer's Toolbox](#c-programmers-toolbox) for details. | |
|
||
1. **Parameters** | ||
* Ordering: Input and then Output. | ||
* All **references** must be **const**, and this is the recommended | ||
way to pass parameters. | ||
* For output parameters, pass pointers. | ||
* Either use const references or pointer for non-fundamental types. The stl types are not fundamental types. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is mainly for consistency, so that when the reader sees a pointer, he/she realizes that the function works by mutating this argument. Care to share the reasons for rewriting this rule? |
||
* Use **const reference** for read only parameter. Use **pointer** for writable parameter. | ||
|
||
2. **Default Argument** | ||
* Not recommended for readability issue. **Do not use** unless | ||
* Not recommended for readability/ABI issue. **Do not use** unless | ||
you have to. | ||
|
||
3. **Trailing Return Type Syntax** | ||
* When in lambda. Period. | ||
* When using template, and the output type should be deducted by input types. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/by/from/ I think this is a good place to have an example :) Our readers may not be equally experienced in C++, and an examples may speak to them better ~ |
||
|
||
4. **Return Value of Complicated Type** | ||
|
||
Returning values of simple types such as `int`, `int64_t`, `bool` | ||
|
@@ -189,12 +201,14 @@ See [C++ Programmer's Toolbox](#c-programmers-toolbox) for details. | |
} | ||
``` | ||
|
||
Also we could return a smart pointer for complex type, such as unique_ptr<std::vector<int>>. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this particular case return unique_ptr<std::vector> is unnecessary and this involves copy elision and/or moving semantics which is the same point as stated in this paragraph :) It is good, however, to add an example demonstrating returning |
||
|
||
I think this pattern should be discouraged in favor | ||
of | ||
[return value optimization](cases/return_value_optimization.md), | ||
because the latter is not only less error-prone, but also **much | ||
more readable**. | ||
|
||
## Exceptions | ||
|
||
1. **DO NOT** throw exceptions. Returns error code, and let the upper | ||
|
@@ -259,6 +273,26 @@ I think for naming we should stick | |
to | ||
[Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html#Naming). | ||
|
||
|
||
## Shared Library/API | ||
|
||
* Please use C-API as the exposed API for most library. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks to me that these paragraphs are about C, not part of the C++ Primer. It looks good to me to keep the content in a separate document. |
||
* Because C-API has a standard ABI in many operating system. All standard C compiler | ||
will compile same C function/variable to same symbol in asm. | ||
|
||
* Try to use void\* for C++ object instance. Try to use function pointer simulate | ||
member function. | ||
|
||
* [Example](cases/c_api_for_cpp.md) | ||
|
||
* If you really want to expose CPP API, the following rules should be followed. | ||
* Do not use virtual method. | ||
* The virtual method will change the method name to an offset in vtable. And the offset is | ||
easily changed if the header could be changed. Then break the ABI compatibility. | ||
* Another guide for virtual method in API is that always creates new virtual method at the END OF CLASS declaration. And do not change older declarations. It will get method as `create`, `createEx`, `createEx2`, `...`, and it seems the virtual method is not necessary for this situations. | ||
* Use [private data class](https://en.wikipedia.org/wiki/Private_class_data_pattern), instead of directly declare member field in private. | ||
|
||
|
||
## C++ Programmer's Toolbox | ||
|
||
There are many available tools for C++, and it is always good to have | ||
|
@@ -277,15 +311,17 @@ format and focus on more important stuff. | |
implementation (e.g. boost) if you have a choice. This helps | ||
reduce both runtime dependencies and compile-time dependencies, | ||
and it promotes readability. | ||
1. Use `const` whenever it makes sense. With C++11, `constexpr` is a | ||
better choice for some uses of `const` | ||
|
||
1. `<stdint.h>` defines types like `int16_t`, `uint32_t`, `int64_t`, | ||
etc. You should always use those in preference to short, unsigned | ||
long long and the like, when you need a guarantee on the size of | ||
an integer. | ||
|
||
1. Macros damage readability. **Do not use** them unless the benefit | ||
is huge enough to compensate the loss. | ||
|
||
1. `auto` is permitted when it promotes readability. | ||
|
||
1. `switch` cases may have scopes: | ||
|
||
```c++ | ||
|
@@ -301,5 +337,30 @@ format and focus on more important stuff. | |
default: { | ||
assert(false); | ||
} | ||
} | ||
} | ||
``` | ||
|
||
1. Use smart_pointer as much as you can. Try unique_ptr rather than shared_ptr. | ||
|
||
* Smart Pointer is a better way to handle memory alloc/free then manually new/delete. | ||
* especially in multi-thread situation. | ||
* unique_ptr is much lighter than shared_ptr. | ||
* shared_ptr maintain a mutex, a reference count. Create and destory a shared_ptr could be a little bit slower than unique_ptr. | ||
* unique_ptr is better for multi-thread condition. It ensures that there are no other thread using this data right now. | ||
* Use unique_ptr<int[] > rather than vector<int> for array that will not grow. | ||
* unique_ptr is easier to return. | ||
* Use weak_ptr to avoid circle reference in shared_ptr. | ||
|
||
1. Use **const** as much as you can. With C++11, `constexpr` is a | ||
better choice for some uses of `const` | ||
|
||
|
||
* It will let compiler to check whether program is correct or not, rather than die in runtime. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/to// I think we need some context on this to make it easier to understand. Care to share a code example? |
||
* Not just for parameter, but also for member function. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you are talking about the function postfix |
||
* **mutalbe** key word may be used in this situation. | ||
|
||
1. Use standard library as much as you can. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
* **Do not** write a sort algorithm except you know what you are doing. For most situation, **std::algorithm** is your friend. And the implementation in **std** is good in general. | ||
* Use **std::thread** rather than pthread. Because C++ std library should be avaliable for most platform. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
In Matrix_API.h | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend renaming the directory from |
||
|
||
```c | ||
#pragma once | ||
|
||
#ifdef __cplusplus | ||
// always add this guard for C++ include | ||
extern "C" { | ||
#endif | ||
|
||
typedef void* Paddle_C_MatrixPtr; | ||
|
||
typedef struct tagPaddle_C_Matrix_API { | ||
void (*zero) (Paddle_C_MatrixPtr* self); | ||
void (*assign) (Paddle_C_MatrixPtr* self, float value); | ||
} Paddle_C_Matrix_API; | ||
|
||
extern Paddle_C_Matrix_API paddle_c_getMatrixAPI(); | ||
|
||
|
||
#ifdef __cplusplus | ||
} | ||
#endif | ||
``` | ||
In Matrix_API.cpp | ||
|
||
```cpp | ||
#include "Matrix_API.h" | ||
|
||
extern "C" { | ||
static void zero_impl...; | ||
|
||
Paddle_C_Matrix_API paddle_c_getMatrixAPI() { | ||
Paddle_C_Matrix_API api; | ||
api.zero = zero_impl; | ||
api.assign = assign_impl; | ||
return api; | ||
} | ||
} | ||
|
||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
We recommend you use C-API to expose your library to others when you are writing CPP library because the ABI of CPP library is very fragile and easily to break. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CPP => C++ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CPP library ==> C++ libraries There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ABI of C++ libraries is fragile and it is easy (for human programmers) to break it. |
||
|
||
When we talk about breaking ABI, we mean when we upgrade our releasing shared library, the applications which depend on old library cannot be run with the new library, and the applications must be recompiled. It will be a disaster if our library is used everywhere. We could always use the static library, to let user always upgrade their applications by recompiling. But, sometimes only shared library can be used, such as multiple language bindings. Or we need user can update their shared library only, without recompiling their application. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when we upgrade a shared library There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the applications => applications |
||
|
||
There are several situations will break the CPP ABI, force users to recompile their applications, or will make binary releasing more difficult when using CPP. | ||
|
||
## User Want to Change Compiler. | ||
|
||
As we all know, the CPP standard didn't contain rules about [name mangling](https://en.wikipedia.org/wiki/Name_mangling). Different compilers would generate different symbols in assembly. It should not be a big thing in Linux/Unix, because the most widely used compilers in Linux/Unix, GCC and Clang, will generate the same symbols in assembly. But in Windows, MSVC use an entirely different strategy about name mangling. So, we should provide at least two binary packages when exposing CPP interface in Windows. | ||
|
||
## Multiple language bindings | ||
|
||
If we want to write our library in C/CPP, but let users can use whatever language(such as Python, Matlab, Julia, Golang) they like. Expose the library in C should be better because most of other languages can invoke C-API in their standard library or as a language built-in feature. C++, especially Modern C++ (C++ 11 and above) is not supported very well for every other language. | ||
|
||
## Change the function definition. | ||
|
||
Modify the definition of a function should break ABI. It is obviously, but it is hard to notice when using CPP. | ||
|
||
Because of function overloading feature, add/remove the parameter of a function (no matter if default argument is added or not) will change the function name in assembly, and break ABI. Here is an example. | ||
|
||
```cpp | ||
// in library v1.0 | ||
void foo(); | ||
|
||
// in library v1.1(wrong) | ||
void foo(bool someFlags=false); | ||
|
||
// in library v1.1(correct) | ||
void foo(); | ||
void fooWithBool(bool someFlags); | ||
``` | ||
|
||
In header above, the calling code of function `foo`, could always be `foo();`, but when upgrading library to v1.1, all application should be recompiled, because the function `foo` is missing. `foo(bool)` is not `foo()` because they have a different name in assembly. | ||
|
||
So never use function overloading and default argument when exposing shared library unless you know what you are doing. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reyoung The
[Expose API/Shared Library]
is a dangling hyperlink.