docs: Add code standard guidelines for #include#1229
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe C++ code standards document refines bracket-spacing guidance and adds an Includes section covering self-contained headers, required include ordering, an example include block, and case-sensitive alphabetical sorting. ChangesC++ Standards Documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cpp-code-standards.md`:
- Line 56: Update the bracket-spacing rule in the C++ standards documentation to
explicitly cover parentheses, square brackets, and braces, rather than using the
ambiguous term “bracket.” Keep the existing guidance about avoiding unnecessary
spaces within these delimiters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e9f551a-e439-418c-93bb-23668f5bf441
📒 Files selected for processing (1)
docs/cpp-code-standards.md
| Trailing whitespace is not allowed. | ||
|
|
||
| You should also not have unneeded spaces within a bracket. | ||
| You should also not have unneeded spaces within a bracket. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify which delimiters this rule covers.
“Bracket” is ambiguous here, while the examples only demonstrate parentheses. Specify parentheses, square brackets, and braces to make the rule unambiguous.
Proposed wording
-You should also not have unneeded spaces within a bracket.
+You should also avoid unneeded spaces inside parentheses, square brackets, and braces.📝 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.
| You should also not have unneeded spaces within a bracket. | |
| You should also avoid unneeded spaces inside parentheses, square brackets, and braces. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cpp-code-standards.md` at line 56, Update the bracket-spacing rule in
the C++ standards documentation to explicitly cover parentheses, square
brackets, and braces, rather than using the ambiguous term “bracket.” Keep the
existing guidance about avoiding unnecessary spaces within these delimiters.
| #include "DBCStores.h" // then project headers, alphabetically | ||
| #include "DatabaseEnv.h" |
There was a problem hiding this comment.
With ASCII sorting? It is essentially sorting "D B C Stores" then "Database Env". D will come before Database.
There was a problem hiding this comment.
We have, to my knowledge, always enforced alphabetical sorting
There was a problem hiding this comment.
It is always alphabetical, but question is whether its case sensitive alphabetical (ASCII) or insensitive. ASCII sort will place DBCStores sort first. Insensitive will treat it both the same as "dbcstores" and "databaseenv", which would place DatabaseEnv first. Other codebase examples similar to what's in ItemEnchantmentMgr.cpp
ConditionMgr.cpp:
#include "ScriptMgr.h"
#include "ScriptedCreature.h"
GroupHandler.cpp:
#include "LFGMgr.h"
#include "Language.h"
Beyond my general observation, I asked Claude to do an actual analysis of sort orders. It did find examples where sorting is case insensitive. GridTerrainLoader.cpp:
#include "Map.h"
#include "MMapMgr.h"
But the vast majority of examples it found (127 vs 5) favoured case sensitive sorting.
| #include "ItemEnchantmentMgr.h" // The file's own header, first | ||
| #include "DBCStores.h" // then project headers, alphabetically | ||
| #include "DatabaseEnv.h" | ||
| #include "Log.h" | ||
| #include "ObjectMgr.h" | ||
| #include "QueryResult.h" | ||
| #include "Timer.h" | ||
| #include "Util.h" | ||
| #include <cmath> // then library headers, alphabetically | ||
| #include <functional> | ||
| #include <vector> |
There was a problem hiding this comment.
Should be project -> lib -> standard C headers
There was a problem hiding this comment.
That would be the Google convention, but I tried to formalize the order based on what AC already uses, which is mostly LLVM. Every single case I found in the codebase, libraries were always in the bottom. Though if you and the other staff think the Google style is what AC should adopt and everything should be eventually changed to match, then yeah sure we can adopt that.
Personally I prefer Google, but I deferred to what's already there.
There was a problem hiding this comment.
isnt the google style lib/standard C at the top?
There was a problem hiding this comment.
Yeah it is. But without exception all the examples I found in AC place libraries at the bottom. Google also breaks the include block into sections https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes but AC uses a single block.
There was a problem hiding this comment.
perhaps I get it mixed and c standard should be before lib. probably doesnt matter
There was a problem hiding this comment.
It's no more than a style choice. AC uses LLVM mostly with libs in the bottom, and its the other way around from Google. Ultimately though, yeah it's mostly about style consistency.
Added guidelines for header file includes and ordering.
2 participants
Description
The codebase is (mostly) consistent in the way includes are formatted. Now there are formal guidelines. Most critically highlighting that headers need to self-contained, but also what order includes should be added in for ease of readability.
Related Issue
Closes
Thank you for contributing to the AzerothCore wiki.
Remember that the wiki is currently available in English and Spanish.
Summary by CodeRabbit
#includeblock order: own header first, then project headers alphabetically, then library headers alphabetically (with an example).