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

[Anton Dukeman via Phabricator via cfe-commits]

adukeman added a comment.

In D155890#4522266 <https://reviews.llvm.org/D155890#4522266>, @ymandel wrote:

> 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.

Is moving these values to config an appropriate task for somebody like me new to working on clang-tidy? I'd be happy to merge this and then try the transition to a config assuming there's some similar examples I can borrow from elsewhere in the codebase.



================
Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:844-846
+      .CaseOfCFGStmt<CXXMemberCallExpr>(
+          isOptionalMemberCallWithName("hasValue"),
+          transferOptionalHasValueCall)
----------------
ymandel wrote:
> 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(...)`.
Sure. I can make that change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890