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