[clang-tools-extra] e7e577f - [clang-tidy] Clarify documention of `bugprone-unchecked-optional-access`.

[Yitzhak Mandelbaum via cfe-commits]

Author: Yitzhak Mandelbaum
Date: 2023-02-10T18:15:21Z
New Revision: e7e577f6842135faaf2c960c7a4e69c71836dc0a

URL: https://github.com/llvm/llvm-project/commit/e7e577f6842135faaf2c960c7a4e69c71836dc0a
DIFF: https://github.com/llvm/llvm-project/commit/e7e577f6842135faaf2c960c7a4e69c71836dc0a.diff

LOG: [clang-tidy] Clarify documention of `bugprone-unchecked-optional-access`.

Removes a reference to google-internal document and expands the relevant material in place.

Fixes: #60633.

Differential Revision: https://reviews.llvm.org/D143750

Added: 
    

Modified: 
    clang-tools-extra/docs/clang-tidy/checks/bugprone/unchecked-optional-access.rst

Removed: 
    


################################################################################
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 256cb6d05e254..ddb43a3dac98b 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
@@ -8,16 +8,14 @@ results. Therefore, it may be more resource intensive (RAM, CPU) than the
 average clang-tidy check.
 
 This check identifies unsafe accesses to values contained in
-``std::optional<T>``, ``absl::optional<T>``, or ``base::std::optional<T>``
-objects. Below we will refer to all these types collectively as
-``optional<T>``.
+``std::optional<T>``, ``absl::optional<T>``, or ``base::Optional<T>``
+objects. Below we will refer to all these types collectively as ``optional<T>``.
 
-An access to the value of an ``optional<T>`` occurs when one of its
-``value``, ``operator*``, or ``operator->`` member functions is invoked.
-To align with common misconceptions, the check considers these member
-functions as equivalent, even though there are subtle 
diff erences
-related to exceptions versus undefined behavior. See
-go/optional-style-recommendations for more information on that topic.
+An access to the value of an ``optional<T>`` occurs when one of its ``value``,
+``operator*``, or ``operator->`` member functions is invoked.  To align with
+common misconceptions, the check considers these member functions as equivalent,
+even though there are subtle 
diff erences related to exceptions versus undefined
+behavior. See *Additional notes*, below, for more information on this topic.
 
 An access to the value of an ``optional<T>`` is considered safe if and only if
 code in the local scope (for example, a function body) ensures that the
@@ -273,3 +271,27 @@ The check does not currently report unsafe optional accesses in lambdas.
 A future version will expand the scope to lambdas, following the rules
 outlined above. It is best to follow the same principles when using
 optionals in lambdas.
+
+Access with ``operator*()`` vs. ``value()``
+-------------------------------------------
+
+Given that ``value()`` has well-defined behavior (either throwing an exception
+or terminating the program), why treat it the same as ``operator*()`` which
+causes undefined behavior (UB)? That is, why is it considered unsafe to access
+an optional with ``value()``, if it's not provably populated with a value?  For
+that matter, why is ``CHECK()`` followed by ``operator*()`` any better than
+``value()``, given that they are semantically equivalent (on configurations that
+disable exceptions)?
+
+The answer is that we assume most users do not realize the 
diff erence between
+``value()`` and ``operator*()``. Shifting to ``operator*()`` and some form of
+explicit value-presence check or explicit program termination has two
+advantages:
+
+  * Readability. The check, and any potential side effects like program
+    shutdown, are very clear in the code. Separating access from checks can
+    actually make the checks more obvious.
+
+  * Performance. A single check can cover many or even all accesses within
+    scope. This gives the user the best of both worlds -- the safety of a
+    dynamic check, but without incurring redundant costs.