chore(): Review process formalization recommendation #3032
Conversation
mariadb-SergeyZefirov
requested review from
drrtuy,
mariadb-LeonidFedorov,
mariadb-DenisKhalikov,
mariadb-AlanMologorsky and
KirillPerov
drrtuy
changed the title
| @@ -0,0 +1,146 @@ | |||
| # Formal review checklist and considerations | |||
|
|
|||
| > "The cost of fixing a defect is proportional to the time between defect introduction and discovery" - main axiom of softwaer engineering. | |||
|
|
||
| The purpose of this document is to list common defects and their sources to prevent their introduction into a code base. | ||
|
|
||
| It is advisory for Columnstore developers to consult this document while doing personal erview of the code to be published. We also should extend this document as new sources of defects come to light. |
There was a problem hiding this comment.
What would be the procedure to introduce changes in the recommendation doc?
| ## Assignment: x = y; | ||
|
|
||
| If x is a std::string and y is a pointer, y is treated as an ASCIIZ string. There is implicit strlen call in there. | ||
|
|
|
|
||
|
|
||
| # Side effects | ||
|
|
There was a problem hiding this comment.
If a function introduces unexpected side-effects they must be explicitly described in the comments so that programmers in future are notified about side-effects.
|
|
||
| ## Inheritance should be shallow | ||
|
|
||
| Liskov substitution principle usually can be realized several ways. Classical example is the Square and Rectangle inheritance - which one should inherit the other? One can argue both ways, that Rectangle adds dimension and should inherit Square and that Square is a constrained Rectangle and thus should be inherited from. |
There was a problem hiding this comment.
Class hierarchy according with Martin enables dependency reversal so it is for the better. You define an iface that tends to change less often and here you go the dependency inversion. And this is C++ in the end. I would say this is too generic to get into this doc.
|
|
||
| ## (De)serialization | ||
|
|
||
| Serialization and deserialization should go through intermediate types. |
There was a problem hiding this comment.
I would put here serdes must use ProtoBuf.
|
@mariadb-LeonidFedorov you should take a look at the doc. |
No description provided.