Skip to content

[clang-tidy] Add a release note about unchecked-optional-access smart pointer caching #122290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Feb 27, 2025

Conversation

jvoung
Copy link
Contributor

@jvoung jvoung commented Jan 9, 2025

With caching added in #120249, the IgnoreSmartPointerDereference option shouldn't be needed anymore.

Other caching also added earlier: #112605

jvoung added 2 commits January 9, 2025 15:05
… pointer caching

With caching added in llvm#120249,
inform in notes that the `IgnoreSmartPointerDereference` option shouldn't
be needed anymore.

Other caching also added earlier:
llvm#112605
@jvoung jvoung marked this pull request as ready for review January 9, 2025 15:31
@llvmbot
Copy link
Member

llvmbot commented Jan 9, 2025

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Jan Voung (jvoung)

Changes

With caching added in #120249, the IgnoreSmartPointerDereference option shouldn't be needed anymore.

Other caching also added earlier: #112605


Full diff: https://github.com/llvm/llvm-project/pull/122290.diff

1 Files Affected:

  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 94e15639c4a92e..47cc4f6f17fed5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -232,6 +232,10 @@ 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``, 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

@jvoung jvoung requested review from 5chmidti and HerrCai0907 January 9, 2025 15:32
Copy link
Contributor

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

does check doc need to be changed also?

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.
@jvoung
Copy link
Contributor Author

jvoung commented Jan 10, 2025

does check doc need to be changed also?

Good question -- I don't see the option IgnoreSmartPointerDereference advertised in the check docs.

Otherwise, we partially updated the check docs about earlier accessor caching. Updated more to mention smart pointer like APIs. Also previously updated internal code comments about IgnoreSmartPointerDereference in the UncheckedOptionalAccessModelOptions struct.

Copy link
Contributor

@EugeneZelenko EugeneZelenko left a comment

Choose a reason for hiding this comment

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

Could you please also move readability-identifier-naming entry before readability-implicit-bool-conversion and place :program:run-clang-tidy.py entry after :program:clang-tidy.py?

@jvoung
Copy link
Contributor Author

jvoung commented Jan 10, 2025

program:run-clang-tidy

Sure thing --

Done separately in #122475 (I kept the "Improved" cluster together, vs moving run-clang-tidy to be after the "Removed" block including the table)

For readability-identifier-naming it looks like @HerrCai0907 already reordered that in c01ffab

@jvoung jvoung merged commit eeb672a into llvm:main Feb 27, 2025
12 checks passed
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Mar 3, 2025
… pointer caching (llvm#122290)

With caching added in llvm#120249,
the `IgnoreSmartPointerDereference` option shouldn't be needed anymore.

Other caching also added earlier:
llvm#112605
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.

6 participants