-
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
Conversation
919b3c7
to
8c7d99a
Compare
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.
I am not an expert on ABIs and may need another pair of eyes to review rules on that. Or rather can you give a write-up on ABIs? That would be awesome :)
* ** 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. | ||
It will breaks the ABIs, make dubug harder. |
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.
s/dubug/debug/
s/breaks/break
* 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 comment
The reason will be displayed to describe this comment to others. Learn more.
s/compiling/compiled/
@@ -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 comment
The 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.
* 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 comment
The 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?
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 comment
The 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 ~
@@ -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 comment
The 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 unique_ptr<SomeType>
where SomeType
is not movable.
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 comment
The 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?
|
||
|
||
* It will let compiler to check whether program is correct or not, rather than die in runtime. | ||
* Not just for parameter, but also for member function. |
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.
So you are talking about the function postfix const
here, right? I think this deserves its own article in the cases
folder, including the discussion on mutable
.
* Not just for parameter, but also for member function. | ||
* **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 comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Sorry I was new to GitHub code review and may have incorrectly disapproved the change ... I think overall it is great just needs some minor change to get rid of the rough edges and maybe adding some more code examples. Thanks a lot! |
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.
Learned a lot. Thank you, @reyoung !
|
||
An exception for this rule is: | ||
|
||
* Use a Private Data Class for exposed API, especially for shared library. |
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.
|
||
## 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 comment
The 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.
@@ -0,0 +1,41 @@ | |||
In Matrix_API.h |
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.
I'd recommend renaming the directory from cases
to c-api
.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
CPP => C++
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
CPP library ==> C++ libraries
@@ -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 comment
The 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.
@@ -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. | |||
|
|||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
when we upgrade a shared library
@@ -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. | |||
|
|||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
the applications => applications
* ** 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. | ||
It will breaks the ABIs, make dubug harder. |
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.
这并不会影响使用我们的shard libraries的应用程序才对吧?因为unnamed namespace里包括的都是应用程序不会调用的符号。
另外,这里的讨论和“break ABI”没有关系吧。所谓break是说如果我们把一个symbol从某个namespace挪到了另一个namespace里,那当然会break ABI,因为这实际上break了API——因为这是给symbol重新起了一个名字了。
No description provided.