From e802465a38c583e72aed205d941b3122a75c2dc9 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Oct 2023 18:08:21 -0400 Subject: [PATCH 1/3] Updated "Class declaration layout" style guideline Developers (mis)interpreted the previous version differently. This version attempts to address known differences while avoiding areas without consensus. Also need to update how overridden methods are declared since Squid started using C++11 "override" specifier. Also synced with the recently added "No new globals" rule. --- .../SquidCodingGuidelines.md | 121 +++++++++++++++--- 1 file changed, 100 insertions(+), 21 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index d217c2e9..640c29c0 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -154,41 +154,120 @@ extern void ReportUsage(ostream &); // global function CamelCased ## Class declaration layout ```c++ -class Foo : public Bar +/// A non-normative illustration of selected layout rules. +/// While usually required, Doxygen member documentation is not shown here. +class Foo { - CBDATA_* or MEMPROXY_* special macro. - public: - all public typedef - - all constructors and operators - Foo destructor (if any) + using A = Bar::B; - /* Bar API */ - all methods overloaded from FooBar parent class + Foo(const char *, size_t); + Foo(Foo &&) = delete; - all public static methods - all public member methods + void print(std::ostream &) const; - all public static variables - all public member variables + const InstanceId id; protected: - all protected static methods - all protected member methods + uint64_t currentMemoryUsage() const; - all protected static variables - all protected member variables + uint64_t maximumMemoryUsage() const; private: - all private static methods - all private member methods + friend class Bar; + + static Storage &Cache(); + + SBuf buffer; +}; +``` + +### Class section order + +Class "sections" order (by member access specifiers): public, protected, +private. Each section, if present, declared once. Omit sections that would be +empty. + +Rationale: List most commonly reused, most important things fist. In this +context, those are public class interfaces. The private section is the least +important implementation detail as far as the notion of a C++ class is +concerned. + +### Class member order + +Within each section, the recommended member order is defined below. + +* friendship declarations +* type and aliases declarations; nested classes/structs/unions +* static member functions +* constructors and assignment operators +* destructors (just one until C++20) +* other non-static member functions except overrides +* overrides (see "Overridden virtual methods" subsection below) +* static data members (most are banned in new code by the "No new globals" rule) +* non-static data members + +Rationale: Group similar things together to facilitate searching and highlight +differences. List things most likely to be reused (by other class members) and +most important/influential things higher. + +### Overridden virtual methods + +Overrides (including `final` declarations) are a special case where we do not +expect member descriptions but do expect a reference to the corresponding API +as sketched below. Overrides are grouped by the (direct or indirect) class +parent that _introduced_ the corresponding API method(s) (i.e. the parent +class that declared the virtual method but could _not_ use an override keyword +in that declaration). That class should describe the virtual method it +introduced. - all private static variables - all private member variables +```C++ +/// Overrides example. +class Derived: public Base +{ +public: + ~Derived() override; + + /* Foo::Grandfather API */ + void sync() override; + void close() override; + + /* Base API */ + void print(std::ostream &) const override; + +protected: + /* Foo::Grandfather API */ + void maintain() override; + void sync() override; }; ``` +Rationale: Provide API context and facilitate searching for member +descriptions without chasing overrides through parents. + +Destructors should be declared with `override` specifier, but are _not_ +grouped with other overrides. + + +### Caveats + +The above rules are not meant to force authors to include any access +specifiers or members that the code does not actually need (except the +"private" specifier should be mentioned explicitly in class declarations that +have only private members -- do not rely on the class default access specifier +being "private"). + +Squid has some legacy code that forces CBDATA-related declarations to be +hoisted to the very top of the class, into the "unnamed" section. Such +hoisting is an exception to the above rules. Eventually, we will stop doing +that, but we should continue doing that for consistency sake until the +offending CBDATA macros are gone. + +Like any style rules, these rules are not comprehensive. If your use case is +not explicitly covered, then look around for similar Squid code and try to be +consistent and/or reasonable. + + ## Member naming Pick one of the applicable styles described below and stick to it. For From 83828e6d0bf7253aaf13ab8aa2f178c9c3d6b095 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Oct 2023 18:13:36 -0400 Subject: [PATCH 2/3] fixup: Improved CBDATA- and MEMPROXY-related coverage ... to the level of previous coverage (at least). --- .../SquidCodingGuidelines.md | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index 640c29c0..dd98c561 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -248,6 +248,23 @@ descriptions without chasing overrides through parents. Destructors should be declared with `override` specifier, but are _not_ grouped with other overrides. +### CBDATA- and MEMPROXY-related declarations + +Squid has some legacy code that forces CBDATA- and MEMPROXY-related +declarations to be hoisted to the very top of the class, into the "unnamed" +section. Such hoisting is an exception to the above rules. Eventually, we will +stop hoisting, but we should continue doing that for consistency sake until we +get rid of the offending macros. + +```C++ +class Uri +{ + MEMPROXY_CLASS(Uri); + +public: + Uri(); +}; +``` ### Caveats @@ -257,12 +274,6 @@ specifiers or members that the code does not actually need (except the have only private members -- do not rely on the class default access specifier being "private"). -Squid has some legacy code that forces CBDATA-related declarations to be -hoisted to the very top of the class, into the "unnamed" section. Such -hoisting is an exception to the above rules. Eventually, we will stop doing -that, but we should continue doing that for consistency sake until the -offending CBDATA macros are gone. - Like any style rules, these rules are not comprehensive. If your use case is not explicitly covered, then look around for similar Squid code and try to be consistent and/or reasonable. From a13fafab2ad562064038affbcae576ea0b12f5a4 Mon Sep 17 00:00:00 2001 From: Alex Rousskov Date: Fri, 6 Oct 2023 18:15:59 -0400 Subject: [PATCH 3/3] fixup: Removed a second empty line to avoid arguing --- docs/DeveloperResources/SquidCodingGuidelines.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/DeveloperResources/SquidCodingGuidelines.md b/docs/DeveloperResources/SquidCodingGuidelines.md index dd98c561..9915aef6 100644 --- a/docs/DeveloperResources/SquidCodingGuidelines.md +++ b/docs/DeveloperResources/SquidCodingGuidelines.md @@ -278,7 +278,6 @@ Like any style rules, these rules are not comprehensive. If your use case is not explicitly covered, then look around for similar Squid code and try to be consistent and/or reasonable. - ## Member naming Pick one of the applicable styles described below and stick to it. For