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

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun 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"),
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:49
+      this);
+  Finder->addMatcher(
+      cxxConstructorDecl(hasAnyConstructorInitializer(
----------------
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? 


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:60
+
+  if (Result.SourceManager->getDiagnostics().hasUncompilableErrorOccurred()) {
+    return;
----------------
Nit: Redundant braces.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UncheckedOptionalAccessCheck.cpp:92
+  for (const SourceLocation &Loc : ExitBlockLattice.getSourceLocations()) {
+    diag(Loc, "unchecked access to optional value");
+  }
----------------
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 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121120