diff --git a/rules/S4999/cfamily/rule.adoc b/rules/S4999/cfamily/rule.adoc index 2d61f9f7c7..cd12ebe65d 100644 --- a/rules/S4999/cfamily/rule.adoc +++ b/rules/S4999/cfamily/rule.adoc @@ -1,54 +1,78 @@ == Why is this an issue? -The functions ``++memcpy++``, ``++memmove++`` and ``++memset++`` can only be used for objects of trivially copyable types. This includes scalar types, arrays, and trivially copyable classes. +The behavior is undefined when `memcpy`, `memmove`, or `memset` are invoked with objects of non-trivially copyable types. +include::../../../shared_content/cfamily/trivially_copyable_types.adoc[] -A class type is trivially copyable if: +== How to fix it -* One or more of the following special member functions is trivial and the rest are deleted: copy constructor, move constructor, copy assignment operator, and move assignment operator, -* It has a trivial, non-deleted destructor, -* It has trivially copyable members and base classes, -* It has no virtual functions. +`memcpy`, `memmove`, and `memset` should only be called on trivially-copyable objects. +Fixing this code defect may require non-trivial refactoring, as illustrated in the following example. -_Note: a default implementation, both explicit (with ``++=default++``) or implicit (if the special member function is omitted), is considered trivial._ +=== Code examples +==== Noncompliant code example -=== Noncompliant code example +Here, `copy` is incorrect because it calls `memcpy` on non-trivial objects. -[source,cpp] +[source,cpp,diff-id=1,diff-type=noncompliant] ---- -class Shape { +class Resource { + Handle* resource_handle; + public: - int x; - int y; - virtual ~Shape(); // This makes the class non trivially copyable + Resource() { + // Acquire the resource handle. + } + + ~Resource() { + // Release the resource handle. + } }; -void f(Shape *dest, Shape *source) -{ - memcpy(dest, source, sizeof Shape); // Noncompliant +void copy(Resource* dest, Resource const* source) { + memcpy(dest, source, sizeof(Resource)); // Noncompliant } ---- +==== Compliant solution -=== Compliant solution +Instead, the `Resource` class can provide a copy assignment operator. -[source,cpp] +[source,cpp,diff-id=1,diff-type=compliant] ---- -class Shape { +class Resource { + Handle* resource_handle; + public: - int x; - int y; - virtual ~Shape(); // This makes the class non trivially copyable + Resource() { + // Acquire the resource handle. + } + + ~Resource() { + // Release the resource handle. + } + + Resource& operator=(Resource const& other) { + // Non-trivial copy assignement: + // copy the handle and update the reference count, etc... + } }; -void f(Shape *dest, Shape *source) -{ - (*dest) = (*source); +void copy(Resource* dest, Resource const* source) { + (*dest) = (*source); } ---- +== Resources +=== Documentation + +* {cpp} reference - https://en.cppreference.com/w/cpp/language/classes#Trivially_copyable_class[Definition of a trivially copyable class]. + +=== Related rules + +* S5000 - "memcmp" should only be called with pointers to trivially copyable types with no padding ifdef::env-github,rspecator-view[] diff --git a/rules/S5000/cfamily/rule.adoc b/rules/S5000/cfamily/rule.adoc index 74c799eeb3..f8ce0aa6c2 100644 --- a/rules/S5000/cfamily/rule.adoc +++ b/rules/S5000/cfamily/rule.adoc @@ -4,17 +4,15 @@ The function ``++memcmp++`` only returns meaningful results for objects of trivi `memcmp` compares the raw memory representation of two objects (what the standard calls their _object representation_). When objects are not trivially copyable or contain padding, they could have different raw memory representations even though they store identical values. So the result of `memcmp` is not meaningful. -A class type is trivially copyable if: +Padding refers to the insertion of additional bits into a structure or class to ensure proper alignment of its members in memory. -* One or more of the following methods is trivial and the rest are deleted: copy constructor, move constructor, copy assignment operator, and move assignment operator, -* It has a trivial, non-deleted destructor. - -An operator/constructor/destructor is trivial if it is not user-provided (it is implicit or defaulted), all base classes and members also have a corresponding trivial operator/constructor/destructor and there are no virtual calls involved. - -Padding is the addition of extra bits to a storage unit to address memory alignment. If the type contains padding, some of its bits might be non-representative. +include::../../../shared_content/cfamily/trivially_copyable_types.adoc[] == How to fix it +The comparison operator `operator==` should be defined and used instead of `memcmp` when the types are not trivially copyable or contain padding. +This allows comparing member by member to check object equality. + === Code examples ==== Noncompliant code example @@ -33,20 +31,6 @@ bool isSame(Shape *s1, Shape *s2) } ---- -[source,cpp,diff-id=2,diff-type=noncompliant] ----- -class Resource { // Not trivially copyable -public: - Ptr* ptr; - ~Resource(); -}; - -bool isSame(Resource *r1, Resource *r2) -{ - return memcmp(r1, r2, sizeof(Resource)) == 0; // Noncompliant -} ----- - ==== Compliant solution [source,c,diff-id=1,diff-type=compliant] @@ -63,6 +47,24 @@ bool isSame(Shape *s1, Shape *s2) } ---- +==== Noncompliant code example + +[source,cpp,diff-id=2,diff-type=noncompliant] +---- +class Resource { // Not trivially copyable +public: + Ptr* ptr; + ~Resource(); +}; + +bool isSame(Resource *r1, Resource *r2) +{ + return memcmp(r1, r2, sizeof(Resource)) == 0; // Noncompliant +} +---- + +==== Compliant solution + [source,cpp,diff-id=2,diff-type=compliant] ---- class Resource { // Not trivially copyable @@ -83,8 +85,13 @@ bool isSame(Resource *r1, Resource *r2) == Resources -https://en.cppreference.com/w/cpp/language/classes#Trivially_copyable_class[Definition of a trivially copyable class]. +=== Documentation +* {cpp} reference - https://en.cppreference.com/w/cpp/language/classes#Trivially_copyable_class[Definition of a trivially copyable class]. + +=== Related rules + +* S4999 - "memcpy", "memmove", and "memset" should only be called with pointers to trivially copyable types ifdef::env-github,rspecator-view[] diff --git a/shared_content/cfamily/trivially_copyable_types.adoc b/shared_content/cfamily/trivially_copyable_types.adoc new file mode 100644 index 0000000000..fc76ede8a4 --- /dev/null +++ b/shared_content/cfamily/trivially_copyable_types.adoc @@ -0,0 +1,15 @@ +Trivially copyable types include: + + * scalar types + * trivially copyable classes + * arrays of these types +// Ignoring cv-qualified versions of these types here for brevity. + +A class is trivially copyable when: + + * all its non-static data members and base classes are trivially copyable types, + * it has no virtual functions or base classes, + * its destructor is trival, + * and one or more of the following special member functions is trivial, and the rest are deleted: copy constructor, move constructor, copy assignment operator, and move assignment operator. + +_Note: a default implementation is always considered trivial, both when it is explicit (with `= default`) or implicit (if the special member function is omitted)._