[PATCH] D121120: [clang-tidy] New check for safe usage of `std::optional` and like types.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:38
+
+  auto HasOptionalCallDescendant = hasDescendant(callExpr(callee(cxxMethodDecl(
+      ofClass(anyOf(hasName("std::optional"), hasName("absl::optional"),
----------------
xazax.hun wrote:
> I am a bit afraid that we have two places to update the list of supported optional types and they can get out of sync. I wonder whether creating a function in `clang/Analysis/FlowSensitive/Models/UncheckedOptionalUseModel.h` to return a matcher that matches the supported optional types is a better approach.
Yes, it is a better approach. :) Done.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:49
+      this);
+  Finder->addMatcher(
+      cxxConstructorDecl(hasAnyConstructorInitializer(
----------------
xazax.hun wrote:
> If we have a `CXXConstructorDecl` where both the initializer and the body has calls to optional types, i.e. both matchers would match, would we process the body of that twice? 
Yes, we would. Good catch. Fixed.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:92
+  for (const SourceLocation &Loc : ExitBlockLattice.getSourceLocations()) {
+    diag(Loc, "unchecked access to optional value");
+  }
----------------
xazax.hun wrote:
> Is there a way in the future to include either the name of the optional or a source range of the access? This can be confusing when we have multiple optionals in the same line. The of course, the column information helps. But the more visual guides we have the better :)
Yes -- we have access to the object expression at the time of detection. So both are reasonable to include. We just need to extend the SourceLocationsLattice to accomodate.  Added a FIXME in the model.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D121120/new/

https://reviews.llvm.org/D121120