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

[Carlos Galvez via Phabricator via cfe-commits]

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