[PATCH] D155890: [clang-tidy] Add folly::Optional to unchecked-optional-access

Fri Jul 21 05:33:11 PDT 2023

ymandel added a comment.

In D155890#4521266 <https://reviews.llvm.org/D155890#4521266>, @carlosgalvezp wrote:

> This should be a configuration option, we should not hardcore project-specific things in the source code.

I agree, but we already are hardcoding specific types -- I think this is a separate (and valid) critique of the design. I'd propose filing an issue on the github tracker and we can follow up there.  I, for one, would love to review such a change but don't have the time to write it.

================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:844-846
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isOptionalMemberCallWithName("hasValue"),
+          transferOptionalHasValueCall)
----------------
A few concerns:
1. This will allow `hasValue` on *any* of the optional types. While we know that the other types don't have this call, this is bad hygiene. At the least, we should note this potential problem in the comments.
2. I don't think its worth duplicating the case above just to change the name, given that the action is identical. Instead, please generalize `isOptionalMemberCallWithName` to take a name matcher, and pass `hasAnyName("has_value", "hasValue")` for this case.  The other calls to `isOptionalMemberCallWithName` will need to be changed to pass just `hasName(...)`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890