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