From 4a8d75386b9fdfac73724b50af06520a52bf3e2e Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Thu, 23 May 2019 19:28:01 -0700 Subject: [PATCH] Clarify/state expected usage of non-const references and 'auto' in coding conventions (#1096) * As we consistently use non-const reference for modifiable arguments that cannot be null, update the conventions to reflect that. Add a note on qualifying 'auto' to make the intent clearer and it easier to notice accidental copies. * Address PR comment by adding a statement around disabling copy/assignment/move for new classes until needed. --- docs/Coding_Conventions_and_Standards.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/Coding_Conventions_and_Standards.md b/docs/Coding_Conventions_and_Standards.md index d1524f4063..f1fce8898a 100644 --- a/docs/Coding_Conventions_and_Standards.md +++ b/docs/Coding_Conventions_and_Standards.md @@ -11,12 +11,18 @@ Google style from https://google.github.io/styleguide/cppguide.html with a few m * Allowed to throw fatal errors that are expected to result in a top level handler catching them, logging them and terminating the program. * Non-const references * Allowed - * However const correctness and usage of smart pointers (shared_ptr and unique_ptr) is expected, so a non-const reference equates to “this is a non-null object that you can change but are not being given ownership of”. + * Use a non-const reference for arguments that are modifiable but cannot be nullptr so the API clearly advertises the intent + * Const correctness and usage of smart pointers (shared_ptr and unique_ptr) is expected. A non-const reference equates to “this is a non-null object that you can change but are not being given ownership of”. * 'using namespace' permitted with limited scope * Not allowing 'using namespace' at all is overly restrictive. Follow the C++ Core Guidelines: * [SF.6: Use using namespace directives for transition, for foundation libraries (such as std), or within a local scope (only)](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rs-using) * [SF.7: Don't write using namespace at global scope in a header file](https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rs-using-directive) +Other +* Qualify usages of 'auto' with 'const', '*', '&' and '&&' where applicable to more clearly express the intent +* When adding a new class, disable copy/assignment/move until you have a proven need for these capabilities. If a need arises, enable copy/assignment/move selectively, and when doing so validate that the implementation of the class supports what is being enabled. + * Use ORT_DISALLOW_COPY_ASSIGNMENT_AND_MOVE initially + * See the other ORT_DISALLOW_* macros in https://github.com/microsoft/onnxruntime/blob/master/include/onnxruntime/core/common/common.h #### Clang-format