Sorry, this change breaks llvm-project/llvm/test/Transforms/InferAddressSpaces/AMDGPU/flat_atomic.ll:

Thank you!

In addition to breaking LLDB's build itself, this change also broke the following test: https://github.com/llvm/llvm-project/blob/main/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string_view/main.cpp that uses std::basic_string<unsigned char> - is it not allowed?

I understand that this is an expedient fix that silences the warning. However, the fundamental problem is the simplistic implementation of the warning (that it is not flow-sensitive), not the inference of nullability. That is, when we are removing nullability qualifiers, we don't know whether there was a check or not. So while this change removes some false positives, it also removes some true positives.

@lukasza I think you forgot to upload an updated version of the patch.

In D155890#4523243, @adukeman wrote:

In D155890#4522266, @ymandel wrote:

In 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.

Thank you! Do you have commit access or do you need me to commit this patch for you?

I'm not sure it is actually an anti-pattern. dyn_cast will transparently handle subclasses, should any be added in the future, but a switch wouldn't.

This commit introduces a null pointer dereference in a unit test, so I'm going to revert it together with all dependent commits:

Using PointerValue to model pointers to data members does not look right to me, because a pointer to data member is an offset within an object that we apply this pointer to, it is not a specific storage location.

LGTM

In D152813#4420448, @mboehme wrote:

We would expect analysis of this code to converge too -- right?

In D152813#4420399, @mboehme wrote:

It looks to me as if the values we're now newly producing for integer literals are causing non-convergence of the analysis on the for-loop.

In D150896#4366698, @philnik wrote:

In D150896#4366612, @gribozavr2 wrote:

In D150896#4366406, @philnik wrote:

Do you possibly have an explicit instantiation of __gnu_cxx::hash_map<std::string>? That would break things with this patch, and is most likely not supported.

Thanks for looking into this! Why would such a use case be unsupported?

http://eel.is/c++draft/requirements#namespace.std-5

In D150896#4366406, @philnik wrote:

Do you possibly have an explicit instantiation of __gnu_cxx::hash_map<std::string>? That would break things with this patch, and is most likely not supported.

The mock optional types in the unit test are just declarations of the API - they don't need any implementations (function or method bodies should be omitted). But the declarations of classes, methods, and functions should mirror the production header closely. There are many seemingly trivial choices one can make when designing an API for a complex type like optional - for example, the specific choice of constructor overload set members, choosing to implement an overload set as a set of concrete functions vs. one function template, SFINAE on any functions, additional defaulted template parameters that the user isn't supposed to set etc. These differences would be hardly noticeable for the majority of "boring" C++ code that uses an optional, but they can be detrimental to tooling's ability to identify calls to the relevant APIs.

Could you change the tests to cover the new case? They are here: llvm-project/clang/unittests/Analysis/FlowSensitive/UncheckedOptionalAccessModelTest.cpp

LGTM too.

LGTM still.

Is it testable? For example, can we show that we don't compute the program state for the program point "p" here:

Nice deduplication and compatibility fixes!

In D147698#4257698, @mboehme wrote:

@gribozavr2 Just to make sure I understand you correctly here (before I make any changes to the code): IIUC you recommend simply doing nothing on a delete expression? (Your arguments for this make sense to me, just want to double-check I understood correctly.)