[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

CS added a comment. Herald added a subscriber: manas. Herald added a project: All. I'm sorry but is this some sort of joke? I don't think we should be calling ChickenFajitas straightaway like this, debounce it, recalculate, repent let's go Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. This patch does not model faithfully how reinterpretcasting member pointers should work. The base specifiers represent the history for the member pointer (aka. mptr), describing

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal, being picky with code from beginners is a good way to train them to write code I think, and so I must thank you for you scrutiny! As for the test you pointed out, it is a wrong test. It is wrong now that I have a better understanding of the problem. There are

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added a comment. I'm really sorry about being sooo picky about this patch. It's not my expertise and the change seems to address a corner-case, so we have to especially careful not introducing bugs. My concern is that I still don't understand why do we want to do anything with reinterp

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. In D96976#2622342 , @steakhal wrote: > I'm somewhat busy. If it's not urgent, I would postpone this. > Ping me in a few weeks. Sure ok. Is it okay if I tell someone else to take a look at this in the meanwhile? Repository: r

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added a comment. In D96976#2622039 , @RedDocMD wrote: > @steakhal, your views? I'm somewhat busy. If it's not urgent, I would postpone this. Ping me in a few weeks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal, your views? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://l

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. Your view, @steakhal? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 328931. RedDocMD added a comment. Replaced smart-ptr dereference with direct access Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/Static

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. In D96976#2605815 , @steakhal wrote: > What is that caveat? The second point in the link I gave above. > Do you have anything specific in mind about implementing the 3rd option? > Why do you think it's //significantly complex//?

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added a comment. In D96976#2605313 , @RedDocMD wrote: > Talking on the mailing list, I got a link on reinterpret_casting for > pointer-to-member: > https://timsong-cpp.github.io/cppwp/n4861/expr.reinterpret.cast#10 > The gist is that this sort

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. Talking on the mailing list, I got a link on reinterpret_casting for pointer-to-member: https://timsong-cpp.github.io/cppwp/n4861/expr.reinterpret.cast#10 The gist is that this sort of cast is only valid if we cast back to the original type (that too with a caveat). N

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:52-62 +void testMultiple() { + int F::*f = &F::field; + int A::*a = reinterpret_cast(f); + int C::*c = reinterpret_cast(f); + A aobj; + C cobj; + aobj.*a = 13; ---

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31 + Some some; + some.*sf = 14; + clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}} RedDocMD wrote: > steakhal wrote: > > RedDocMD wrot

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31 + Some some; + some.*sf = 14; + clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}} steakhal

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:29-31 + Some some; + some.*sf = 14; + clang_analyzer_eval(some.*sf == 14); // expected-warning{{UNKNOWN}} RedDocMD wrote: > steakhal wrote: > > The assignmen

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:20 int DoubleDerived::*ddf = &Base::field; int Base::*bf = reinterpret_cast(reinterpret_cast(reinterpret_cast(ddf))); Base ba

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:43-50 +struct A {}; +struct B : public A {}; +struct C { + int field; +}; +struct D : public C {}; +struct E : public B, public D {};

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. I have serious concerns inline. Comment at: clang/test/Analysis/reinterpret-cast-pointer-to-member.cpp:20 int DoubleDerived::*ddf = &Base::field; int Base

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326733. RedDocMD added a comment. Removed unnecessary includes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/StaticAnalyzer/Core/PathSen

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal, you are absolutely right! It works. Thank you for pointing it out, not sure how I missed this earlier this evening. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326731. RedDocMD added a comment. Replaced BFS with existing CXXRecordDeclMethod Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/StaticAna

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added a comment. In D96976#2590200 , @RedDocMD wrote: > Unfortunately, all the methods on CXXRecordDecl, like the one you mentioned, > are for querying and thus returns a `bool`, while I need the entire path. AFAIK the second overload accepts an

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. > So here you are implying that for reinterpret casts, the > `CastE->path_begin()-end()` range is empty? And that is why you need to > manually come up with a //possible// CXXBaseSpecifier list? @steakhal, Yes. For illustration, the following code: struct Base {

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added a comment. In D96976#2587513 , @RedDocMD wrote: > Many thanks for you comments, @steakhal! > I will address the issues you have pointed out in this comment. To clean > things up I should perhaps add some more clarification to the summary. >

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326414. RedDocMD added a comment. Added some more comments to increase clarity Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/StaticAnaly

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326413. RedDocMD added a comment. Added test for multiple inheritance Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/StaticAnalyzer/Core/

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326410. RedDocMD added a comment. Cleaned up tests, added test above class hierarchy Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 Files: clang/include/clang/Stati

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD updated this revision to Diff 326393. RedDocMD marked 2 inline comments as done. RedDocMD added a comment. Removed explicit for-loop with range-for loop Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D969

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD marked 2 inline comments as done. RedDocMD added a comment. Many thanks for you comments, @steakhal! I will address the issues you have pointed out in this comment. To clean things up I should perhaps add some more clarification to the summary. > Could you elaborate on what approach did

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

steakhal added a comment. In D96976#2585498 , @RedDocMD wrote: > @steakhal, could you please review this? The longer I stare at it the more questions I have. However, I don't know how member pointers work in the analyzer and I'm not really in the mood to

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @steakhal, could you please review this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @vsavchenko, could you please review this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD added a comment. @vsavchenko, does it look okay? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96976/new/ https://reviews.llvm.org/D96976 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96976: [analyzer] Fix reinterpret_cast handling for pointer-to-member

RedDocMD created this revision. RedDocMD added reviewers: vsavchenko, NoQ, dcoughlin. Herald added subscribers: steakhal, ASDenysPetrov, martong, Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun. RedDocMD requested review of this revision.