[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added a comment. In D95877#2563383 , @davezarzycki wrote: > The test seems to be broken on Fedora 33 (x86-64 with clang-11): > > XPASS: Clang :: Analysis/reinterpret-cast-pointer-to-member.cpp (6531 of > 74360) > TEST

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. In D95877#2563383 , @davezarzycki wrote: > The test seems to be broken on Fedora 33 (x86-64 with clang-11): > > XPASS: Clang :: Analysis/reinterpret-cast-pointer-to-member.cpp (6531 of > 74360) > TEST

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

davezarzycki added a comment. The test seems to be broken on Fedora 33 (x86-64 with clang-11): XPASS: Clang :: Analysis/reinterpret-cast-pointer-to-member.cpp (6531 of 74360) TEST 'Clang :: Analysis/reinterpret-cast-pointer-to-member.cpp' FAILED

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. In D95877#2563220 , @RKSimon wrote: > @RedDocMD Please can you take a look at this buildbot failure: > http://lab.llvm.org:8011/#/builders/124 Yes. Thanks for the ping  Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RKSimon added a comment. @RedDocMD Please can you take a look at this buildbot failure: http://lab.llvm.org:8011/#/builders/124 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG21daada95079: [analyzer] Fix static_cast on pointer-to-member handling (authored by RedDocMD, committed by vsavchenko). Repository: rG LLVM

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko accepted this revision. vsavchenko added a comment. This revision is now accepted and ready to land. Great job! Thanks for dealing with all my nit-picking! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. @vsavchenko, anything else to add? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877 ___ cfe-commits mailing list

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. In D95877#2548027 , @vsavchenko wrote: > Yes, it'd be better to extract this case into a separate file and mark it as > XFAIL Done that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD updated this revision to Diff 322094. RedDocMD added a comment. Moved failing test of reinterpret_cast to its own file, marked to failx Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877 Files:

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added a comment. In D95877#2548021 , @RedDocMD wrote: > In D95877#2547949 , @vsavchenko > wrote: > >> I think we had a bit of misunderstanding about the test. It still should >> pass (we can't have

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. In D95877#2547949 , @vsavchenko wrote: > I think we had a bit of misunderstanding about the test. It still should > pass (we can't have broken tests, it will disrupt CI bots). Try to put the > test case, so it is THERE and

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added a comment. In D95877#2547303 , @RedDocMD wrote: > @vsavchenko are there some more changes you want done? I think we had a bit of misunderstanding about the test. It still should pass (we can't have broken tests, it will disrupt CI

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. @vsavchenko are there some more changes you want done? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877 ___ cfe-commits mailing list

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:184 + llvm::SmallPtrSet BaseSpecSeen; + for (const auto : BaseSpecList) { +auto BaseType = BaseSpec->getType(); vsavchenko wrote: > vsavchenko wrote: > > LLVM

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD updated this revision to Diff 321816. RedDocMD marked 10 inline comments as done. RedDocMD added a comment. Some more aesthetic cleanup, added a failing test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

ASDenysPetrov added a comment. I downloaded and tested this patch. On the whole it works OK. See my suggestions below. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:205 + llvm::SmallPtrSet BaseSpecSeen; + for (const auto : BaseSpecList) { +auto

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:180 +bool noRepeatedElements( +const llvm::ImmutableList ) { RedDocMD wrote: > vsavchenko wrote: > > nit: functions represent actions, in languages verbs act

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:180 +bool noRepeatedElements( +const llvm::ImmutableList ) { vsavchenko wrote: > nit: functions represent actions, in

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:180 +bool noRepeatedElements( +const llvm::ImmutableList ) { nit: functions represent actions, in languages verbs act the same way, so it's recommended to

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD marked 2 inline comments as done. RedDocMD added a comment. @vsavchenko , does it look okay now? Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:27 #include +#include #include vsavchenko wrote: > We don't need this anymore :) Wow,

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD updated this revision to Diff 321714. RedDocMD marked an inline comment as done. RedDocMD added a comment. Added TODO for reinterpret_cast, moved assertion code to its own block Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:210 + "repeated elements"); +BaseSpecSeen.insert(BaseType); } Containers like `map` and `set` usually return a pair for `insert`, where the second

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added a comment. In D95877#2542578 , @RedDocMD wrote: > @vsavchenko, after some experimentation, I found out that handling > reinterpret_cast will be a real pain. Consider the following: > > struct Base { > int field; > }; > >

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. @vsavchenko, after some experimentation, I found out that handling reinterpret_cast will be a real pain. Consider the following: struct Base { int field; }; struct Derived : public Base {}; struct DoubleDerived : public Derived {}; struct

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD updated this revision to Diff 321476. RedDocMD added a comment. Updated to use new function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877 Files:

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD marked 2 inline comments as done. RedDocMD added a comment. @vsavchenko , I have used the logic that you suggested and put in a better assertion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD updated this revision to Diff 321139. RedDocMD added a comment. Cleaner implementation of the popping logic, using STL/LLVM algorithms Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877 Files:

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:226 +llvm::make_range(BaseList.begin(), BaseList.end()), +[](const auto ) -> bool { return I.second; }); + RedDocMD wrote: > vsavchenko wrote: > > I

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:226 +llvm::make_range(BaseList.begin(), BaseList.end()), +[](const auto ) -> bool { return I.second; }); + vsavchenko wrote: > I think it's all a bit too

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added a comment. Also, all these `PathRange` and `PathList` don't really reflect what they stand for semantically and talk more about implementation. It's not the problem of this patch, but it doesn't make your logic easier to comprehend. Comment at:

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added a comment. @vsavchenko could you please see the current logic if it is better now? Comment at: clang/test/Analysis/pointer-to-member.cpp:304 + grandpa.field = 10; + int Grandfather::*gpf1 = static_cast(sf); + int Grandfather::*gpf2 =

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD updated this revision to Diff 321078. RedDocMD added a comment. Made assertion more verbose Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/ https://reviews.llvm.org/D95877 Files:

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD updated this revision to Diff 321074. RedDocMD added a comment. Remoevd the use of std::list with llvm::SmallVector. Removed the need to delete elements. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95877/new/

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

RedDocMD added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:223 + auto DelIt = find_first(BaseList.begin(), BaseList.end(), PathBase); + assert(DelIt != BaseList.end() && "PTM has insufficient base specifiers"); +

[PATCH] D95877: [analyzer] Fix static_cast on pointer-to-member handling

vsavchenko added a comment. Thanks for taking your time and getting to the root cause of it! Comment at: clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp:209 +// thing about Clang AST. +std::list BaseList; +for (const auto : PathList)