[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

lebedev.ri added a comment. In D56322#1389013 , @labath wrote: > Btw, I've just noticed that the files you've added here still have the old > license header. Would be good to get at least an automatic Herald rule for this I suspect there might be more o

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. Btw, I've just noticed that the files you've added here still have the old license header. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 ___ lldb-com

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL353324: [Reproducers] SBReproducer framework: Capture & Replay (authored by JDevlieghere, committed by ). Herald added a p

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere added a comment. In D56322#1387357 , @labath wrote: > This looks good to me. Thank you for your patience. My pleasure, thank you for the thorough review! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llv

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. This looks good to me. Thank you for your patience. Comment at: lldb/source/Utility/ReproducerInstrumentation.cpp:91 +Recorder::~Recorder() { + assert(m_result_recorded && "Did you forget SB_RECORD_RESULT?"); + UpdateBoundary(); old ma

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 185585. JDevlieghere marked 7 inline comments as done. JDevlieghere added a comment. Feedback Pavel. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 Files: lldb/include/lldb/API/SBReproducer.h lldb/

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. I think we're very close. This batch of comments is just about small improvements to the tests. It looks like the `Validate` thingy wasn't such a clever idea, since the fact that the framework creates extra copies means that there are some unvalidated copies floating aro

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 185431. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 Files: include/lldb/API/SBReproducer.h include/lldb/Utility/ReproducerInstrumentation.h source/API/CMakeLists.txt source/API/SBReproducer.cp

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added inline comments. Comment at: unittests/Utility/ReproducerInstrumentationTest.cpp:399-404 + EXPECT_EQ(foo.g_a, 100); + EXPECT_EQ(foo.g_b, 200); + EXPECT_TRUE(Equals(foo.g_c, c)); + EXPECT_EQ(foo.g_d, "bar"); + EXPECT_TRUE(Equals(foo.g_e, e)); + EXPECT_EQ(foo.g_f

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. Thank you. I like the new test, but see my comment about avoiding duplicating macros in the test. Besides returning objects, I can think of a couple of more interesting scenarios to cover: - recursive calls (to exercise the g_global_boundary thingy) - having two objects

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere added a comment. (I plan to add another test that deals with returning objects) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 185208. JDevlieghere added a comment. Add context CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 Files: include/lldb/API/SBReproducer.h include/lldb/Utility/ReproducerInstrumentation.h source/API

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 185207. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 Files: include/lldb/API/SBReproducer.h include/lldb/Utility/ReproducerInstrumentation.h source/API/CMakeLists.txt source/API/SBReproducer.cp

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere marked 11 inline comments as done. JDevlieghere added a comment. In D56322#1382971 , @labath wrote: > Thank you. I think this looks much better now. > > It occurred to me that the (de)serializer classes are fully standalone. Since > they alre

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. Thank you. I think this looks much better now. It occurred to me that the (de)serializer classes are fully standalone. Since they already have a comprehensive test suite, do you think it would make sense to split them off into a separate patch, which can be committed sep

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 184874. JDevlieghere added a comment. - Move framework in Utility - Update tests CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 Files: include/lldb/API/SBReproducer.h include/lldb/Utility/Reproduce

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere added a comment. In D56322#1381254 , @labath wrote: > In D56322#1380996 , @JDevlieghere > wrote: > > > In D56322#1380442 , @labath wrote: > > > > > I have a lot

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 184825. JDevlieghere marked 27 inline comments as done. JDevlieghere added a comment. Address Pavel's inline comments, modulo - Fixing the ODR violation (should be easier to verify inline comments are addressed before moving the code). - Fixing the endi

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere added inline comments. Comment at: source/API/SBReproducerPrivate.h:82-93 +/// Base class for tag dispatch used in the SBDeserializer. Different tags are +/// instantiated with different values. +template struct SBTag {}; + +/// We need to differentiate between poin

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. In D56322#1380996 , @JDevlieghere wrote: > In D56322#1380442 , @labath wrote: > > > I have a lot of comments. The two major ones are: > > > > - i think the way you link the tests is in the U

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere added a comment. In D56322#1380442 , @labath wrote: > I have a lot of comments. The two major ones are: > > - i think the way you link the tests is in the UB territory. I explain this > in detail in one of the inline comments. Thanks, you'r

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. I have a lot of comments. The two major ones are: - i think the way you link the tests is in the UB territory. I explain this in detail in one of the inline comments. - I believe that your unit tests (not just in this patch) focus too much on testing the behavior of a si

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 184644. JDevlieghere added a comment. Add unit testing for supporting classes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 Files: include/lldb/API/SBReproducer.h source/API/CMakeLists.txt sourc

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere added a comment. I split off the framework (this revision) and the addition of the macros (D57475 ). I plan on updating the patch with tests, but I believe it's ready for review in the meantime. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

JDevlieghere updated this revision to Diff 184367. JDevlieghere retitled this revision from "SBReproducer prototype" to "[Reproducers] SBReproducer framework". JDevlieghere edited the summary of this revision. JDevlieghere added reviewers: labath, davide, aprantl, zturner, jingham. JDevlieghere se