[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett created this revision. Herald added subscribers: atanasyan, jrtc27. Herald added a project: All. DavidSpickett requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. In future I want to extend RegisterInfo and that will likely be non

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett added reviewers: aprantl, clayborg. DavidSpickett added a comment. Note that this does not add any non trivial types but of course I did test it by adding some to RegisterInfo. This work is early parts of https://discourse.llvm.org/t/rfc-showing-register-fields-in-lldb/64676. =

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

tschuett added a comment. If I read your summary correctly, then `std::variant` would simplify your code. LLVM still uses `llvm::optional`. As there is no LLVM variant, I would go for `std::variant` . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett added a comment. > As there is no LLVM variant, I would go for std::variant I was under the impression that we can use 17 language features but not library features, yet. Let me check that what the minimum toolchains have. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett added a comment. Variant is supported from: libstdc++ 7.1 VS 2017 15.0 libcxx 4.0 So far so good relative to https://llvm.org/docs/GettingStarted.html#host-c-toolchain-both-compiler-and-standard-library. https://en.cppreference.com/w/cpp/compiler_support/17#C.2B.2B17_library_featu

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

tschuett added a comment. Sorry. I hoped that variant would make your life easier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134041/new/ https://reviews.llvm.org/D134041 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

labath added a comment. I think the main reason that the RegisterInfo struct is such as it is, is because it wants to be trivially constructible (or whatever the c++ term for that is). I'm pretty sure that adding a std::string member will make it stop being that, and I don't think we suddenly w

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

clayborg added a comment. Looks fine to me as this is all NFC with a look toward the future. Only one questions about if we want to switch the EmulateInstruction::GetRegisterInfo to return an optional as well? Comment at: lldb/source/Plugins/Instruction/ARM/EmulateInstructio

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

clayborg added a comment. I do share the concern that Pavel has where currently RegisterInfo does not require global constructors, and it would be good to keep it this way. Right now RegisterInfo is POD which means if there is a global array of RegisterInfo structs, it gets written into a data

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett added a comment. > Sorry. I hoped that variant would make your life easier. Np, one can dream (until the next out of reach c++ feature comes along). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134041/new/ https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett added a comment. > I think the main reason that the RegisterInfo struct is such as it is, is > because it wants to be trivially constructible (or whatever the c++ term for > that is). I'm pretty sure that adding a std::string member will make it stop > being that, and I don't thin

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett added inline comments. Comment at: lldb/include/lldb/Core/EmulateInstruction.h:196 + struct RegisterPlusOffsetStruct { RegisterInfo reg; // base register int64_t signed_offset; // signed offset added to base register laba

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

labath added a comment. In D134041#3805034 , @DavidSpickett wrote: >> That said I would *love* is someone changed the RegisterInfo structs into >> something saner, but I think that will need to be more elaborate than simply >> stuffing a std::vector me

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

jasonmolenda added a comment. In D134041#3805941 , @labath wrote: > In D134041#3805034 , @DavidSpickett > wrote: > >>> That said I would *love* is someone changed the RegisterInfo structs into >>> something sane

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

labath added a comment. In D134041#3806994 , @jasonmolenda wrote: > In D134041#3805941 , @labath wrote: > >> In D134041#3805034 , >> @DavidSpickett wrote: >> That s

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

clayborg added a comment. In D134041#3811289 , @labath wrote: > In D134041#3806994 , @jasonmolenda > wrote: > >> In D134041#3805941 , @labath wrote: >> >>> In D134041#380

[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

DavidSpickett abandoned this revision. DavidSpickett added a comment. https://reviews.llvm.org/D135015 landed using std::variant and no one has complained so far, so this whole patch is now irrelevant. If it comes back, I'll just use variant. I agree that there are reasons to keep things POD so