Skip to content

[common] enforce correct CRTP usage for mix-in classes - #11880

Merged
jwhui merged 1 commit into
openthread:mainfrom
abtink:clearable/force-correct-crtp
Aug 29, 2025
Merged

[common] enforce correct CRTP usage for mix-in classes#11880
jwhui merged 1 commit into
openthread:mainfrom
abtink:clearable/force-correct-crtp

Conversation

@abtink

@abtink abtink commented Aug 28, 2025

Copy link
Copy Markdown
Member

The mix-in helper classes like Clearable<T>, Equatable<T>, and Unequatable<T> are intended for CRTP style inheritance, where T is the derived class itself. A mistaken inheritance, such as class Foo : public Clearable<Bar>, can compile successfully but lead to subtle bugs.

This change enforces the correct CRTP usage at compile time. By making the constructors of these helper classes private and declaring the derived template class T as a friend, any incorrect inheritance will now result in a build failure. This approach correctly detects such a mistake, even if Foo and Bar happen to be friends of each other.

Additionally, Equatable<T> is updated to provide both operator== and operator!=, removing its dependency on Unequatable<T>. This change allows us to apply the private constructor enforcement to Equatable<T> as well.

@github-actions

github-actions Bot commented Aug 28, 2025

Copy link
Copy Markdown

Merging #11880 into main

name branch text data bss total
ot-cli-ftd 064bd3b 479608 860 66548 547016
be2218c 479608 860 66548 547016
+/- +0 +0 +0 +0
ot-ncp-ftd 064bd3b 445324 764 61792 507880
be2218c 445324 764 61792 507880
+/- +0 +0 +0 +0
ot-cli-mtd 064bd3b 372352 764 51020 424136
be2218c 372352 764 51020 424136
+/- +0 +0 +0 +0
ot-ncp-mtd 064bd3b 353452 764 46288 400504
be2218c 353452 764 46288 400504
+/- +0 +0 +0 +0
ot-cli-ftd-br 064bd3b 581376 868 135332 717576
be2218c 581376 868 135332 717576
+/- +0 +0 +0 +0
ot-rcp 064bd3b 63136 568 20804 84508
be2218c 63136 568 20804 84508
+/- +0 +0 +0 +0
Library files
name branch text data bss total
libopenthread-ftd.a 064bd3b 246826 95 40326 287247
be2218c 246826 95 40326 287247
+/- +0 +0 +0 +0
libopenthread-cli-ftd.a 064bd3b 60843 0 8083 68926
be2218c 60843 0 8083 68926
+/- +0 +0 +0 +0
libopenthread-ncp-ftd.a 064bd3b 33257 0 5948 39205
be2218c 33257 0 5948 39205
+/- +0 +0 +0 +0
libopenthread-mtd.a 064bd3b 165183 0 24822 190005
be2218c 165183 0 24822 190005
+/- +0 +0 +0 +0
libopenthread-cli-mtd.a 064bd3b 41134 0 8059 49193
be2218c 41134 0 8059 49193
+/- +0 +0 +0 +0
libopenthread-ncp-mtd.a 064bd3b 25785 0 5948 31733
be2218c 25785 0 5948 31733
+/- +0 +0 +0 +0
libopenthread-ftd-br.a 064bd3b 350317 100 109078 459495
be2218c 350317 100 109078 459495
+/- +0 +0 +0 +0
libopenthread-cli-ftd-br.a 064bd3b 78409 0 8115 86524
be2218c 78409 0 8115 86524
+/- +0 +0 +0 +0
libopenthread-rcp.a 064bd3b 9932 0 5060 14992
be2218c 9932 0 5060 14992
+/- +0 +0 +0 +0
libopenthread-radio.a 064bd3b 19423 0 238 19661
be2218c 19423 0 238 19661
+/- +0 +0 +0 +0

@codecov

codecov Bot commented Aug 29, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.64%. Comparing base (064bd3b) to head (ff003b4).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11880      +/-   ##
==========================================
+ Coverage   74.25%   74.64%   +0.39%     
==========================================
  Files         643      645       +2     
  Lines      100095    95803    -4292     
==========================================
- Hits        74323    71515    -2808     
+ Misses      25772    24288    -1484     
Files with missing lines Coverage Δ
src/core/common/clearable.hpp 100.00% <ø> (+20.00%) ⬆️
src/core/common/equatable.hpp 100.00% <100.00%> (ø)

... and 387 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@abtink
abtink force-pushed the clearable/force-correct-crtp branch from 023ef43 to 32a7534 Compare August 29, 2025 15:43
@abtink
abtink marked this pull request as ready for review August 29, 2025 15:44
@abtink

abtink commented Aug 29, 2025

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly enforces CRTP usage for the mix-in helper classes by making their constructors private and friending the derived type. This is a great change that will prevent subtle bugs from incorrect inheritance by catching them at compile time. The refactoring of Equatable<T> to remove its dependency on Unequatable<T> is also a clean improvement. The changes are logical and well-executed. I have one minor suggestion to improve a comment for better clarity.

Comment thread src/core/common/equatable.hpp Outdated
The mix-in helper classes like `Clearable<T>`, `Equatable<T>`, and
`Unequatable<T>` are intended for CRTP style inheritance, where `T`
is the derived class itself. A mistaken inheritance, such as `class
Foo : public Clearable<Bar>`, can compile successfully but lead to
subtle bugs.

This change enforces the correct CRTP usage at compile time. By making
the constructors of these helper classes `private` and declaring the
derived template class `T` as a `friend`, any incorrect inheritance
will now result in a build failure. This approach correctly detects
such a mistake, even if `Foo` and `Bar` happen to be `friend`s of
each other.

Additionally, `Equatable<T>` is updated to provide both `operator==`
and `operator!=`, removing its dependency on `Unequatable<T>`. This
change allows us to apply the `private` constructor enforcement to
`Equatable<T>` as well.
@abtink
abtink force-pushed the clearable/force-correct-crtp branch from 32a7534 to ff003b4 Compare August 29, 2025 15:47
@jwhui
jwhui requested a review from Copilot August 29, 2025 19:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enforces correct CRTP (Curiously Recurring Template Pattern) usage for mix-in helper classes by making their constructors private and using friend declarations. This prevents compile-time errors from incorrect inheritance patterns where the template parameter doesn't match the derived class.

Key changes:

  • Added private constructors and friend declarations to Clearable<T>, Equatable<T>, and Unequatable<T>
  • Removed Equatable<T>'s inheritance from Unequatable<T> and implemented operator!= directly
  • Enhanced compile-time safety for CRTP pattern enforcement

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/core/common/equatable.hpp Added CRTP enforcement to Unequatable<T> and Equatable<T>, removed inheritance relationship
src/core/common/clearable.hpp Added CRTP enforcement to Clearable<T> with private constructor and friend declaration

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/core/common/equatable.hpp
@jwhui
jwhui merged commit 853bbd1 into openthread:main Aug 29, 2025
129 of 130 checks passed
@bukepo

bukepo commented Sep 12, 2025

Copy link
Copy Markdown
Member

Is there another mistaken inheritance case that a class having the Clearable mix-in is inherited by another class? It which would cause the similar problem the PR is trying to prevent, right?

@abtink

abtink commented Sep 12, 2025

Copy link
Copy Markdown
Member Author

Is there another mistaken inheritance case that a class having the Clearable mix-in is inherited by another class? It which would cause the similar problem the PR is trying to prevent, right?

Yes. Exactly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants