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