diff --git a/_docs_v7/Code-Review.md b/_docs_v7/Code-Review.md index 56f760bf..b55a1347 100644 --- a/_docs_v7/Code-Review.md +++ b/_docs_v7/Code-Review.md @@ -19,19 +19,19 @@ All developers and users (internal and external) are encouraged to participate i 2. Is the change implemented in the correct way? - Does it interact minimally with the rest of the code? - Does it have the correct algorithmic complexity? -- Is it located in the place? (file, etc.) +- Is it located in the right place? (file, class, etc.) 3. Is the code legible? - Is the code mostly legible on its own without documentation? - Are the variable names concise and accurate? -- Is there documentation where necessary, and it is correct? +- Is there documentation where necessary, and is it correct? 4. Does the code follow established conventions? - Does it match the SU2 code style? - Do the variable names follow the same patterns as in other parts of the code? # Good code changes -The above list is a long list of questions. A large change to the code will be much harder to review than a small change. As such, good pull requests will contain a minimal set of changes to be useful. Pull requests should "do one thing". In some cases, "doing one thing" may be a large change to the code, such as adding a new flow solver. In most cases, changes can be done in small increments that can each individually be reviewed and evaluated. Pull requests should not be of the form "Add a new config option and fix a bug in interation_structure.cpp". These should be two separate pull requests. +The above list is a long list of questions. A large change to the code will be much harder to review than a small change. As such, good pull requests will contain a minimal set of changes to be useful. Pull requests should "do one thing". In some cases, "doing one thing" may be a large change to the code, such as adding a new flow solver. In most cases, changes can be done in small increments that can each individually be reviewed and evaluated. Pull requests should not be of the form "Add a new config option and fix a bug in interation_structure.cpp". **Such pull requests will be required to be split**. # The Code Review Process Github provides an easy interface for performing code reviews as part of every Pull Request. After a Pull Request is submitted to the SU2 'develop' branch, two different developers must review and approve the code changes before the request can be merged, in addition to passing the Travis CI regression test suite. Reviewers have the opportunity to comment on the changes and requests specific changes. @@ -40,6 +40,6 @@ In response to these comments, the pull requester should make changes to the cod When a reviewer is happy with the proposed changes to the code, the reviewer should approve and can say "LGTM", standing for "looks good to me". In general, the changes to the code should be finalized before a LGTM is given, though if there are only very minor outstanding issues an LGTM can be given along with the changes. For example, if the only outstanding issue with the PR is that a word has been misspelled, a reviewer may make an inline comment about the misspelling, and in the main dialogue say "LGTM with the comment fix". -All developers are encouraged to participate in the code review process, as the code is for everyone. However, there will typically be a specific set of developers who are experts in the section of code that is being modified. Generally, an LGTM should be gotten from at least one of these developers before merging. Users can be requested using "@username", for example, "PTAL @su2luvr". This sends that user an email about the pull request. Similarly, this can be used to request the opinions of other developers. While this can feel burdensome, it is in place to maintain good, correct code. Please use good judgement -- if the change is a spelling fix in a comment, it is not necessary to solicit the opinion of the entire development team. +**All developers are encouraged to participate in the code review process, as the code is for everyone.** However, there will typically be a specific set of developers who are experts in the section of code that is being modified. Generally, a LGTM should be gotten from at least one of these developers before merging. Users can be requested using "@username", for example, "PTAL @su2luvr". This sends that user an email about the pull request. Similarly, this can be used to request the opinions of other developers. While this can feel burdensome, it is in place to maintain good, correct code. Once the proper set of "LGTM"s has been received, the change can be merged. If the pull-requester has commit access, it is tradition to let them make the merge. If the requester does not, then the main/final reviewer can submit the merge. If the pull-request came from an internal branch, the branch should be deleted on conclusion if it is no longer useful. diff --git a/_docs_v7/Code-Structure.md b/_docs_v7/Code-Structure.md index 4a00cbf4..b4271b4b 100644 --- a/_docs_v7/Code-Structure.md +++ b/_docs_v7/Code-Structure.md @@ -5,6 +5,8 @@ permalink: /docs_v7/Code-Structure/ Full details on the class hierarchy and internal structure of the code can be found in the Doxygen documentation for SU2. A brief description for the major C++ classes is given on this page. +**Note:** The images below can be out of sync with the current version of the code. + The objective of this section is to introduce the C++ class structure of SU2 at a high level. The class descriptions below focus on the structure within SU2_CFD (the main component of SU2), but many of the classes are also used in the other modules. Maximizing the flexibility of the code was a fundamental driver for the design of the class architecture, and an overview of the collaboration diagram of all classes within SU2_CFD is shown below. ![Class Structure General](../../docs_files/class_c_driver__coll__graph.png) diff --git a/_docs_v7/Gitting-Started.md b/_docs_v7/Gitting-Started.md index cc08f795..93a3fdbf 100644 --- a/_docs_v7/Gitting-Started.md +++ b/_docs_v7/Gitting-Started.md @@ -16,6 +16,10 @@ We follow [a popular git branching strategy](http://nvie.com/posts/a-successful- ## Contributing Code -SU2 merges new code contributions through pull requests. As a new developer, you'll want to fork SU2 to your personal account. This creates a clone of the whole SU2 repository, branches and all, inside your github account. Generally you'll want to start from the develop branch, but you can check with the developers if you think it would be more appropriate to work on a feature branch. +SU2 merges new code contributions through pull requests. As a new developer, you'll want to fork SU2 to your personal account. This creates a clone of the whole SU2 repository, branches and all, inside your github account. Generally you'll want to **start from the develop branch**, but you can check with the developers if you think it would be more appropriate to work on a feature branch (join the SU2 slack channel or open a GitHub discussion to get in touch). -You can push all of your working changes to your forked repository. Once you're happy with these, and want to push them to the origin repository, submit a pull request to the 'develop' branch under the SU2 repo. Make sure to pull any new changes from the origin repository before submitting the pull request, so that the changes can be merged more simply. The SU2 developers will review the changes, make comments, ask for some edits. Then when everything looks good, your changes will merge into the main development branch! +You can push all of your working changes to your forked repository. Once you're happy with these, and want to push them to the origin repository, submit a pull request to the 'develop' branch under the SU2 repo. Make sure to **pull any new changes regularly** from the origin repository before submitting the pull request, so that the changes can be merged more simply. The SU2 developers will review the changes, make comments, ask for some edits, and when everything looks good, your changes will merge into the main development branch! + +### General Guidelines +- If you have a very clear plan for the work you are doing, open a **draft pull request** so that the maintainers can start reviewing early. +- If you are working on a large feature (1k+ lines of code (loc), or 200+ loc with changes to 10+ files) ask to be added to the SU2 organization to work from the SU2 repo directly instead of your fork (this makes reviewing easier). diff --git a/_docs_v7/Running-Regression-Tests.md b/_docs_v7/Running-Regression-Tests.md index 333bfe68..a35faff7 100644 --- a/_docs_v7/Running-Regression-Tests.md +++ b/_docs_v7/Running-Regression-Tests.md @@ -3,17 +3,7 @@ title: Running Regression Tests permalink: /docs_v7/Running-Regression-Tests/ --- -The regression tests can be run on your local machine by using the Python scripts available in the SU2/TestCases/ directory and the mesh files from the su2code/TestCases repository. See the [Test Cases](/docs_v7/Test-Cases/) page for more information on working with the TestCases repo. +The regression tests can be run on your local machine by using the Python scripts available in the SU2/TestCases/ directory and the mesh files from the su2code/TestCases and Tutorials repository. See the [Test Cases](/docs_v7/Test-Cases/) page for more information on working with the TestCases repo. -When you are ready to combine your modifications into the develop branch, creating a pull request will automatically run these same regression tests on the Travis CI system. Your pull request must pass these tests in order to be merged into the develop branch. In the pull request, you will be able to see the state of the tests, and by clicking on links find the details of the test results. +When you are ready to combine your modifications into the develop branch, creating a pull request will automatically run these same regression tests. Your pull request must pass these tests in order to be merged into the develop branch. In the pull request, you will be able to see the state of the tests, and by clicking on links find the details of the test results. -If you are working with a forked version of the repository, you can use the following directions to run these same regression tests within your repository rather than within the su2code/ repository. This is preferable if you are not ready to submit your code to the develop branch and just want to run the tests, or if you want to create your own regression tests. - -1. Modify the travis.yml file in the develop branch to update the ***email address*** and ***repository url***. At this point you can also modify which branch will have the tests run. Commit the change to your fork/develop. -2. Sign up for Travis CI and allow access to your account. -3. Activate the repository within Travis CI. -4. Modify the "README" file in the SU2/ directory such that the url points to the results for your fork rather than su2code/SU2. -5. Commit the result into your fork/develop. -6. View the results: when you open up your fork/develop on github the readme file should display. There will be a colored link going to the travis CI build which state whether the test is building, passing, or failing. This link will lead to the details of the tests. Pull requests between your fork/develop and any branches you have created on your fork will also run regression tests. - -If the tests do not run at first, double check that the repository is activated within Travis CI, and if so push a commit with some small change to the travis.yml file to your repository. If it still doesn't work, double check your urls and refer to Travis CI help menu. diff --git a/_docs_v7/Style-Guide.md b/_docs_v7/Style-Guide.md index 11f02921..90c36b2b 100644 --- a/_docs_v7/Style-Guide.md +++ b/_docs_v7/Style-Guide.md @@ -3,101 +3,69 @@ title: Style Guide permalink: /docs_v7/Style-Guide/ --- -SU2 is released under an open source license to facilitate its widespread use and development in the scientific computing community. To support uniformity and consistency in the style of the source code, a C++ style guide has been included on this page, and it is strongly encouraged that outside developers adhere to the guidelines dictated in the style guide to maintain readability of the source. +To support uniformity and consistency in the style of the source code, a C++ style guide has been included on this page, and it is strongly encouraged that outside developers adhere to the guidelines dictated in the style guide to maintain readability of the code. Any contributions from the scientific community at-large are encouraged and welcomed. Feel free to contact the development team at any time. -This document describes the conventions that will be used when implementing new features in SU2. This includes allowed syntactic and semantic language features, filename conventions, indentation conventions and more. The consistency is fundamental, it is very important that any programmer be able to look at another part of the code and quickly understand it, the uniformity in the style is a key issue. Some of the ideas expressed in this document comes from the Google C++ Style Guide (revision 3.188). +This document describes the conventions that will be used when implementing new features in SU2. Consistency is fundamental, it is very important that any programmer be able to look at another part of the code and quickly understand it. Some of the ideas expressed in this document come from the Google C++ Style Guide, others from the [C++ Core Guidelines](https://github.com/su2code/SU2/issues/1218). ## C++ style guide +Below is summary of the rules we try to follow in SU2. **Older parts of SU2 are not good examples of these rules.** + +**Duplicating code is absolutely not allowed.** + ### Version numbering -Each code of the SU2 suite must have a release number following the rule Major.Patch, where the Major number is increased each time a new major update is performed and the Patch number is increased each time new features are added. The configuration file also has a number following the rule Major.Patch, where Major correspond with the SU2_CFD major version and Patch is increased with new changes. +The SU2 suite has a major release number followed the rule minor.path, where the minor number is increased each time a significant update is performed and the patch number is increased in maintenance releases. +Patch releases cannot break backward compatibility. ### Standard conformance, and formatting -Source code must comply with the C++ ISO/ANSI standard. with respect to the formatting some recommendation can be made: -- Each line of text in your code should be at most 80 characters long. -- Non-ASCII characters should be rare, and must use UTF-8 formatting. -- Use only spaces (default indent is 2 spaces). You can set your editor to emit spaces when you hit the tab key. -- Sections in public, protected and private order, each indented one space. -- The hash mark that starts a preprocessor directive should always be at the beginning of the line. -- When you have a boolean expression that is longer than the standard line length, be consistent in how you break up the lines. +SU2 is written for C++11, the formatting rules are defined in a `clang-format` file located in the root of the repository. +**New files must follow the formatting rules exactly.** ### Files, functions, and variables -Here some basic recommendation are made for creating files, functions, and variables: +Basic recommendations for creating files, functions, and variables: - C++ filenames must have extension .cpp. - C++ header filenames must have extension .hpp. In general, every .cpp file should have an associated .hpp file. -- C++ inline filenames must have extension .inl. Define functions inline only when they are small, say, 10 lines or less. -- All subprograms (subroutines of functions) must be contained in a class. Each parent class must be contained in a file with the same name as the class (plus extension ’.cpp’, and ’.hpp’). This implies that there can only be one parent class per file. -- When defining a function, parameter order is: inputs, then outputs. +- When defining a function, the parameter order is: inputs, then outputs. - Order of includes. Use standard order for readability and to avoid hidden dependencies: C library, C++ library, other libraries', your project's. -- Prefer small and focused functions. +- Write small and focused functions. - Use overloaded functions (including constructors) only if a reader looking at a call site can get a good idea of what is happening without having to first figure out exactly which overload is being called. - Local variables. Place a function's variables in the narrowest scope possible, and initialize variables in the declaration. -- Static or global variables of class type are forbidden: they cause hard-to-find bugs due to indeterminate order of construction and destruction. -- In the initialization, use 0 for integers, 0.0 for reals, NULL for pointers, and '\0' for chars. - -### Classes - -The classes are the key element of the object oriented programming, here some basic rules are specified. -In general, constructors should merely set member variables to their initial values. Any complex initialization should go in an explicit Init() method. -- You must define a default constructor, and destructor. -- Use the C++ keyword explicit for constructors with one argument. -- Use a struct only for passive objects that carry data; everything else is a class. -- Do not overload operators except in rare, special circumstances. -- Use the specified order of declarations within a class: public: before private:, methods before data members (variables), etc. - -### Syntactic and semantic requirements - -In this section you can find some basic rules for programming: -- All allocated memory must be deallocated at program termination. -- Read or write operations outside an allocated memory block are not allowed. -- Read or write outside index bounds in arrays or character variables are not allowed. -- No uninitialized/undefined values may be used in a way that could affect the execution. -- Local variables that are not used must be removed. -- Pointer variables must be initialized with NULL unless they are obviously initialized in some other way before they are used. -- Indentation will be two steps for each nested block-level. -- In the header file, at the beginning of each program unit (class, subroutine or function) there must be a comment header describing the purpose of this code. The doxygen format should be used. -- When possible, it is better to use #DEFINE with a physical meaning to simplify the code. -- The code must be compiled using doxygen to be sure that there is no warning in the commenting format. -- When describing a function the following tag must be used: \brie_, \para_\[in\], \para_\[out\], \retur_, \overload. -- Static or global variables of class type are forbidden: they cause hard-to-find bugs due to indeterminate order of construction and destructionUse 0 for integers, 0.0 for reals, NULL for pointers, and '\0' for chars. -- All parameters passed by reference must be labeled const. We strongly recommend that you use const whenever it makes sense to do so. -- In the code short, int, and the unsigned version of both must be used case depending. -- Code should be 64-bit and 32-bit friendly. Bear in mind problems of printing, comparisons, and structure alignment - -### Naming - -The most important consistency rules are those that govern naming. The style of a name immediately informs us what sort of thing the named entity is: a type, a variable, a function, a constant, a macro, etc., without requiring us to search for the declaration of that entity. - -The following naming conventions for variables must be used: -- Geometry: Normal, Area (2D, and 3D), Volume (2D, and 3D), Coord, Position. Solution: Solution, Residual, Jacobian. -- Function names, variable names, and filenames should be descriptive; eschew abbreviation. Types and variables should be nouns, while functions should be "command" verbs. -- Elementary functions that set or get the value of a variable (e.g. Number) must be called as GetNumber(), or GetNumber(). Function names start with a capital letter and have a capital letter for each new word, with no underscores. -- Variable names are all lowercase, with underscores between words. -- The name for all the classes must start with the capital "C" letter, followed by the name of the class (capitalizing the first letter), if the name is composed by several words, all the words must be together, e.g.: CPrimalGrid. -- All the variables that are defined in a class must be commented using /\*< \brief \________.\*/. +- Static or global variables of class type are forbidden. +- Make abundant use of `const`. +- Always use `su2double` for floating point variables, do not use `double`, `float`, or `auto`. +- Use `auto` or `auto&` or `auto*` for other types of variables. -### Comments +### Code Documentation -The documentation, and comments must be Doxygen friendly, here I include some basic features: +Classes and functions must be documented with doxygen: - Start each file with a copyright notice, followed by a description of the contents of the file. -- Every class definition should have an accompanying comment that describes what it is for and how it should be used. -- Declaration comments describe use of the function; comments at the definition of a function describe operation. -- In general the actual name of the variable should be descriptive enough to give a good idea of what the variable is used for. +- Every class/function definition must have an accompanying comment that describes what it is for and how it should be used. +- The code must be compiled using doxygen to be sure that there are no warnings in the commenting format. +- When describing a function or class the following tags must be used: `\brief`, `\param[in]`, `\param[out]`, `\return`, `\overload`. +- Do not document the obvious, but rather the expected behavior/usage or the class or function, use `\note` to add details of an algorithm, include links to literature when appropriate. - In your implementation you should have comments in tricky, non-obvious, interesting, or important parts of your code. - Pay attention to punctuation, spelling, and grammar; it is easier to read well-written comments than badly written ones. - Short, and long comments must be in inside of /\*--- (your comment here) ---\*/, and they must be located just before the line to be commented. - Math comments are welcome and should be in the Latex language. -### Debugger tools +### Naming + +The consistency boat sailed a long time ago in SU2, the information below is more descriptive than prescriptive. -- The C++ code must support the following features for debugging: -- Array index bounds may be checked at runtime. -- Conformance with C++ may be checked. -- Use of obsolescent features may be reported as compilation warnings. -- Unused variables may be reported as compilation warnings. +The following naming conventions are in use: +- Function names, variable names, and filenames should be descriptive; eschew abbreviation. Types and variables should be nouns, while functions should be "command" verbs. +- Elementary functions that set or get the value of a variable (e.g. Number) must be called as SetNumber(), or GetNumber(). Function names start with a capital letter and have a capital letter for each new word, with no underscores. +- The name for all the classes must start with the capital "C" letter, followed by the `NameOfTheClass` (camel case). - Iteration: iPoint, jPoint, kPoint, iNode, jNode, kNode, iElem, jElem, kElem, iDim, iVar, iMesh, iEdge. + +**Variable names** +- Acceptable styles are `lowerCamelCase` and `snake_case`. +- Member variables can be `CamelCase`, or be ended `with_underscore_`. +- Template parameters are `CamelCase`. +- Be consistent and follow the existing style if you are modifying or fixing code, do not increase entropy... +