[clang-tools-extra] [clang-tidy] Add a release note about unchecked-optional-access smart pointer caching (PR #122290)
Jan Voung via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 14 10:47:24 PST 2025
https://github.com/jvoung updated https://github.com/llvm/llvm-project/pull/122290
>From 342ff1a05caa6943fe8a86415f30b433ac30106f Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Thu, 9 Jan 2025 14:10:30 +0000
Subject: [PATCH 1/5] [clang-tidy] Add a release note about
unchecked-optional-access smart pointer caching
With caching added in https://github.com/llvm/llvm-project/pull/120249,
inform in notes that the `IgnoreSmartPointerDereference` option shouldn't
be needed anymore.
Other caching also added earlier:
https://github.com/llvm/llvm-project/pull/112605
---
clang-tools-extra/docs/ReleaseNotes.rst | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 94e15639c4a92e..f325b9ecbe1547 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -232,6 +232,9 @@ Changes in existing checks
<clang-tidy/checks/bugprone/unchecked-optional-access>` to support
`bsl::optional` and `bdlb::NullableValue` from
<https://github.com/bloomberg/bde>_.
+ Fixed false positives from smart pointer accessors repeated in checking
+ ``has_value`` and accessing ``value`` are now fixed by now caching.
+ So the option `IgnoreSmartPointerDereference` should no longer be needed.
- Improved :doc:`bugprone-unhandled-self-assignment
<clang-tidy/checks/bugprone/unhandled-self-assignment>` check by fixing smart
>From 5ca75af3d62de29313032b7673106ac3fcd0d9f7 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Thu, 9 Jan 2025 15:20:00 +0000
Subject: [PATCH 2/5] Clean up text
---
clang-tools-extra/docs/ReleaseNotes.rst | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index f325b9ecbe1547..47cc4f6f17fed5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -233,8 +233,9 @@ Changes in existing checks
`bsl::optional` and `bdlb::NullableValue` from
<https://github.com/bloomberg/bde>_.
Fixed false positives from smart pointer accessors repeated in checking
- ``has_value`` and accessing ``value`` are now fixed by now caching.
- So the option `IgnoreSmartPointerDereference` should no longer be needed.
+ ``has_value`` and accessing ``value``, by caching the locations returned
+ by the accessors. The option `IgnoreSmartPointerDereference` should no
+ longer be needed.
- Improved :doc:`bugprone-unhandled-self-assignment
<clang-tidy/checks/bugprone/unhandled-self-assignment>` check by fixing smart
>From a40a7f598ddfb155e456aa8adaaf013b059e11c1 Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Fri, 10 Jan 2025 14:20:42 +0000
Subject: [PATCH 3/5] Also update checker docs a bit
We updated some earlier, but not the later section about recommending
binding to a local variable. Also did not update w.r.t. smart pointer
like APIs.
---
.../bugprone/unchecked-optional-access.rst | 23 +++++++++++--------
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
index 815b5cdeeebe24..aed04dd500a996 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -71,8 +71,8 @@ For example:
.. code-block:: c++
void f(Foo foo) {
- if (foo.opt().has_value()) {
- use(*foo.opt()); // unsafe: it is unclear whether `foo.opt()` has a value.
+ if (foo.take().has_value()) {
+ use(*foo.take()); // unsafe: it is unclear whether `foo.take()` has a value.
}
}
@@ -81,10 +81,12 @@ Exception: accessor methods
The check assumes *accessor* methods of a class are stable, with a heuristic to
determine which methods are accessors. Specifically, parameter-free ``const``
-methods are treated as accessors. Note that this is not guaranteed to be safe
--- but, it is widely used (safely) in practice, and so we have chosen to treat
-it as generally safe. Calls to non ``const`` methods are assumed to modify
-the state of the object and affect the stability of earlier accessor calls.
+methods and smart pointer-like APIs (non ``const`` overloads of ``*`` when
+there is a parallel ``const`` overload) are treated as accessors. Note that
+this is not guaranteed to be safe -- but, it is widely used (safely) in
+practice, and so we have chosen to treat it as generally safe. Calls to non
+``const`` methods are assumed to modify the state of the object and affect
+the stability of earlier accessor calls.
Rely on invariants of uncommon APIs
-----------------------------------
@@ -191,14 +193,15 @@ paths that lead to an access. For example:
Stabilize function results
~~~~~~~~~~~~~~~~~~~~~~~~~~
-Since function results are not assumed to be stable across calls, it is best to
-store the result of the function call in a local variable and use that variable
-to access the value. For example:
+Function results are not assumed to be stable across calls, except for
+const accessor methods. For more complex accessors (non-const, or depend on
+multiple params) it is best to store the result of the function call in a
+local variable and use that variable to access the value. For example:
.. code-block:: c++
void f(Foo foo) {
- if (const auto& foo_opt = foo.opt(); foo_opt.has_value()) {
+ if (const auto& foo_opt = foo.take(); foo_opt.has_value()) {
use(*foo_opt);
}
}
>From 032e66275d801db3e6ccab8db142b5db300c760c Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Mon, 13 Jan 2025 20:18:04 +0000
Subject: [PATCH 4/5] Simplify note a bit more
---
.../clang-tidy/checks/bugprone/unchecked-optional-access.rst | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
index aed04dd500a996..552e6db6996966 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst
@@ -84,9 +84,8 @@ determine which methods are accessors. Specifically, parameter-free ``const``
methods and smart pointer-like APIs (non ``const`` overloads of ``*`` when
there is a parallel ``const`` overload) are treated as accessors. Note that
this is not guaranteed to be safe -- but, it is widely used (safely) in
-practice, and so we have chosen to treat it as generally safe. Calls to non
-``const`` methods are assumed to modify the state of the object and affect
-the stability of earlier accessor calls.
+practice. Calls to non ``const`` methods are assumed to modify the state of
+the object and affect the stability of earlier accessor calls.
Rely on invariants of uncommon APIs
-----------------------------------
>From f82528b580ff347557feb29cb79ac9ede4e9ad5f Mon Sep 17 00:00:00 2001
From: Jan Voung <jvoung at gmail.com>
Date: Tue, 14 Jan 2025 18:42:48 +0000
Subject: [PATCH 5/5] Remove part about caching, and clarify that option will
be removed.
---
clang-tools-extra/docs/ReleaseNotes.rst | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 81a633e8a3de93..b66539c5cd6e87 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -240,9 +240,9 @@ Changes in existing checks
``bsl::optional`` and ``bdlb::NullableValue`` from
<https://github.com/bloomberg/bde>_.
Fixed false positives from smart pointer accessors repeated in checking
- ``has_value`` and accessing ``value``, by caching the locations returned
- by the accessors. The option `IgnoreSmartPointerDereference` should no
- longer be needed.
+ ``has_value`` and accessing ``value``. The option
+ `IgnoreSmartPointerDereference` should no longer be needed and will be
+ removed.
- Improved :doc:`bugprone-unhandled-self-assignment
<clang-tidy/checks/bugprone/unhandled-self-assignment>` check by fixing smart
More information about the cfe-commits
mailing list