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

Fri Jul 21 10:32:28 PDT 2023

gribozavr2 added a comment.

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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155890