I think configurable option is needed instead of hard code function name for non-std libarary.

@HerrCai0907 I am happy to do so! Are there any examples how it looks for other clang-tidy checks?

Also, do you want to do it in scope of this PR or as a separate one?
Since non-std function names (e.g.hasValue) are also processed forfolly::Optional in code above my change.

Thanks for review!

You can look atno-malloc check for similar option with function names.

You can add function-lists both forbdlb and other libraries. The code must be very similar.

@vbvictor, thank you for the help! I checked the implementation ofno-malloc configuration, and it does make sense to me.

Just want to double-check expectation, do we want to have a configuration like this, with just method names (name of options is subject to change):

NonStdMakeValue: makeValue;makeValueInplace.// BloombergLP::bdlb:NullableValueNonStdHasValue: hasValue.// folly::OptionalNonStdNoValue: isNull.// BloombergLP::bdlb:NullableValue

Or do we want to specify which class method belongs to:

NonStdMakeValue: BloombergLP::bdlb:NullableValue::makeValue;BloombergLP::bdlb:NullableValue::makeValueInplace.NonStdHasValue: folly::Optional::hasValue. NonStdNoValue: BloombergLP::bdlb:NullableValue::isNull.

For me, the first version looks better because:

• It's easier
• It makes it possible to easily enable this check for more "make value" custom methods in classes inherited from the optional.

However, it's less intuitive to me than option two because different non-standard methods belong to different classes. But this is how it works now, so maybe we could build on top of it, just with method names, and then improve it or extend it.

I'd prefer 2nd option because we have more control over what method belong to what class. Imagine this situation:

structoptional1() {hasNull();// check that it has value but it's nullptrisNull();// check that it has no value inside}structoptional2() {hasNull();// check that it has no value inside}

If we go with 1st option, we would writeNonStdNoValue: hasNull;isNull, but that would produce errors becauseoptional1::hasNull doesn't actually check for no value,optional1::isNull does.
To express intentional behavior, we need to writeNonStdNoValue: optional2::hasNull;optional1::isNull.

But if it works like option1 right now, I don't oppose implementing option1.
I suppose we could harden it in the future if ever needed. My example withoptional1 andoptional2 wouldn't likely appear in real life IMO.

I started experimenting with implementing the mentioned configuration and have a doubt that I would like to talk about.

What is the preferred way to passUncheckedOptionalAccessModelOptions intoUncheckedOptionalAccessModel?

Currently, the existing configuration (IgnoreSmartPointerDereference) is being passed only intoUncheckedOptionalAccessDiagnoser, which is not what I need in my case.

If I understand correctly, passingOptions intoModel requires changes toDataflowAnalysis.h.

The "cheapest" solution might be to implement code like thishere:

template<typename AnalysisT,typename OptionsT>autocreateAnalysis(ASTContext &ASTCtx, Environment &Env, OptionsT &Options)    -> decltype(AnalysisT(ASTCtx, Env, OptionsT)) {returnAnalysisT(ASTCtx, Env, Options);}

Plus, modify the code that calls it and extend the constructor of theUncheckedOptionalAccessModel.

But,theFIXME comment nearby highlights that this approach is rather a hack and should be refactored later.

I also see that there isDataflowAnalysisContext::Options, but I am not sure if it's super useful for my case.

@vbvictor would appreciate your feedback!

I'm not familiar with dataflow framework and I can't give a good direction for implementing this.
Addedymand as one of the previous reviewers of the code andXazax-hun,gribozavr as maintainers of analysis framework.
They may share an opinion on this matter.

I'll defer to@ymand on the final decision what is the best way to do this. But in my opinion, this make a strong argument that the analysis instance should always be created by the client (e.g., we should pass the analysis instance todiagnoseFunction and similar) as all analyses might need different configuration options and we definitely don't want to deal with all that in a generic way in the framework.