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 <jvo...@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 <jvo...@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 <jvo...@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 <jvo...@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 <jvo...@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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
