-
Notifications
You must be signed in to change notification settings - Fork 6
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
docs(cpp): Add partial set of C++ contribution guidelines. #26
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe changes involve the addition of comprehensive guidelines for C++ development in the project's documentation. A new section detailing automated linting processes and adherence to Google's C++ style guide has been incorporated into Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
docs/dev-guide/contrib-guides-cpp.md (4)
7-8
: Grammar improvement neededConsider revising for clarity and proper article usage:
-As mentioned in the overview, where possible, we have automated linting processes so you need not +As mentioned in the overview, where possible, we have automated linting processes so you need not +remember all the guidelines perfectly.🧰 Tools
🪛 LanguageTool
[uncategorized] ~8-~8: You might be missing the article “the” here.
Context: ... processes so you need not remember all guidelines perfectly. For C++, we currently use th...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
13-18
: Consider adding a comma after 'since'For better readability and proper Canadian English punctuation:
-When the linter disagrees with a guideline, err on the side of following the linter since: +When the linter disagrees with a guideline, err on the side of following the linter, since:🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: Possible missing comma found.
Context: ...eline, err on the side of following the linter since: * we don't want automated runs ...(AI_HYDRA_LEO_MISSING_COMMA)
42-43
: Consider expanding the header file guidanceThe guidance about header file extensions could benefit from additional context:
-* Header files should end in `.hpp` -* Don't use non-header files meant for inclusion since they can confuse static analysis tools. +* Header files should end in `.hpp` (not `.h`) to clearly indicate C++ headers +* Don't use non-header files meant for inclusion, as they can confuse static analysis tools and + make dependencies less clear
47-47
: Consider adding an example for the #define guard formatThe format description would be clearer with a concrete example:
-* The format of the symbol name should be `<NAMESPACE>_<FILENAME-STEM>_<FILENAME-EXTENSION>` +* The format of the symbol name should be `<NAMESPACE>_<FILENAME-STEM>_<FILENAME-EXTENSION>` + Example: For a file "string_utils.hpp" in namespace "yscope": + `YSCOPE_STRING_UTILS_HPP`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
docs/dev-guide/contrib-guides-cpp.md
(1 hunks)docs/dev-guide/contrib-guides-overview.md
(1 hunks)docs/dev-guide/index.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/dev-guide/contrib-guides-cpp.md
[uncategorized] ~8-~8: You might be missing the article “the” here.
Context: ... processes so you need not remember all guidelines perfectly. For C++, we currently use th...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~13-~13: Possible missing comma found.
Context: ...eline, err on the side of following the linter since: * we don't want automated runs ...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
docs/dev-guide/index.md (1)
10-10
: LGTM! The C++ guidelines entry is properly integrated.
The new entry is correctly positioned within the contribution guidelines section and maintains consistent formatting with existing entries.
docs/dev-guide/contrib-guides-overview.md (1)
14-18
: LGTM! The C++ grid item card is properly formatted.
The new grid item maintains consistency with the existing layout and properly links to the C++ guidelines.
docs/dev-guide/contrib-guides-cpp.md (1)
83-88
: Verify link accessibility
Please ensure all external links are accessible and point to stable versions:
✅ Verification successful
All external links are accessible and valid
All referenced links return HTTP 200 status codes, indicating they are accessible and point to valid resources:
- Y-Scope development utilities documentation and configuration files
- Google C++ Style Guide and its specific sections
- Referenced commit in the Google Style Guide repository
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all referenced links are accessible
# Check each external link
for url in \
"https://github.com/y-scope/yscope-dev-utils/blob/main/docs/lint-tools-cpp.md" \
"https://github.com/y-scope/yscope-dev-utils/blob/main/lint-configs/.clang-format" \
"https://github.com/y-scope/yscope-dev-utils/blob/main/lint-configs/.clang-tidy" \
"https://google.github.io/styleguide/cppguide.html" \
"https://github.com/google/styleguide/commit/8f97e24da04753c7a15eda6b02114a01ec3146f5"
do
curl -s -L -I "$url" | head -n 1
done
Length of output: 2550
docs/dev-guide/contrib-guides-cpp.md
Outdated
|
||
#### Doing work in constructors | ||
|
||
* We allow (but discourage) the use of exceptions, even in constructors. |
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.
🛠️ Refactor suggestion
Clarify exception handling guidance
The current wording about exceptions is ambiguous:
-* We allow (but discourage) the use of exceptions, even in constructors.
+* We allow exceptions in constructors, but discourage their use unless absolutely necessary.
+ When exceptions are used in constructors, ensure:
+ * The exception conditions are well-documented
+ * Resources are properly managed (RAII)
+ * The class maintains a consistent state
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
* We allow (but discourage) the use of exceptions, even in constructors. | |
* We allow exceptions in constructors, but discourage their use unless absolutely necessary. | |
When exceptions are used in constructors, ensure: | |
* The exception conditions are well-documented | |
* Resources are properly managed (RAII) | |
* The class maintains a consistent state |
* For C headers that have C++ counterparts (e.g., `stddef.h` vs `cstddef`), prefer the C++ | ||
counterpart. |
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.
Shall we enforce this since the C++ version may have some extern "C"
wrappers that ensure the linkage correctness?
|
||
#### Doing work in constructors | ||
|
||
We allow (but discourage) the use of exceptions, even in constructors. |
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.
Shall we mention using factory functions as the suggested practice to handle errors in construction?
|
||
#### Declaration order | ||
|
||
Group static constants/methods/members together before non-static constants/methods/constants. |
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 it's better to list the ordering with more details:
- constants (should be static)
- types (alias + new defined types, no rules on the internal orderings since one symbol name may depend the other)
- static methods
- factory functions
- other static methods
- static member variables
- constructor
- copy/move constructors and operators
- destructors
- methods
- derived methods
- overloaded operators
- methods
- member variables
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.
didn't mention friend
though since it's almost not used in our code base
Description
This PR starts a new contribution guidelines section for C++. This is not the complete set of our C++ contribution guidelines, but is a starting point. We will update it in future PRs until complete.
Validation performed
task serve
Summary by CodeRabbit
New Features
Documentation