carlosgalvezp added a comment. In D155890#4523262 <https://reviews.llvm.org/D155890#4523262>, @gribozavr2 wrote:
> In D155890#4523243 <https://reviews.llvm.org/D155890#4523243>, @adukeman > wrote: > >> 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. > > I think it can be a good starter task for a new engineer on the project. > However, don't underestimate this problem, it will require the code to be > refactored a little bit. For example, the function `hasOptionalClassName` > needs restructuring so that it can accept class names from a list. Not a lot > of work, but it isn't mechanically replacing string literals with a variable > either. Indeed this is not "the standard" CT check, the core is part of Clang so I think it'd be good to add reviewers there as well in case this affects other parts of the codebase. In that sense it does not seen as trivial as I thought to make this user configurable, so perhaps opening a ticket and solve it there is a faster way forward. ================ Comment at: clang/lib/Analysis/FlowSensitive/Models/UncheckedOptionalAccessModel.cpp:62 + return N != nullptr && (isTopLevelNamespaceWithName(*N, "base") || + isTopLevelNamespaceWithName(*N, "folly")); } ---------------- If there's no need for `absl` here, why do we need to add `folly`? 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
