labath added a comment. In D133906#3791352 <https://reviews.llvm.org/D133906#3791352>, @JDevlieghere wrote:
> In D133906#3791153 <https://reviews.llvm.org/D133906#3791153>, @jingham wrote: > >> This patch makes me a little sad because it breaks the "Jump to Definition" >> chain in Xcode (and I bet it does in other IDE's that support this.) You >> used to be able to do "Jump to Definition" on ProcessSP, then jump to >> definition on the class name in the shared pointer definition to jump to the >> class. Now the first jump takes you to: >> >> LLDB_FORWARD_CLASS(Process) >> >> in the lldb-forward.def file, and you can't go any further because the IDE >> can't tell what to do with the contents of the .def file (it has no way to >> know how it was imported to create real definitions). These .def insertions >> almost always make things harder to find in the actual code, so they aren't >> free. > > The alternative would be to have tablegen generate the header, but I think > that's overkill, and I'm not even sure Xcode would pick the generated header. I have a feeling that the code would be simpler if you reversed the "iteration order", and it might even make the go-to definition command more useful. I'm thinking of something like // no .def file #define LLDB_FORWARD_CLASS(cls) \ namespace lldb_private { class cls; } \ namespace lldb { using cls##SP = std::shared_ptr<lldb_private::cls>; } \ ... LLDB_FORWARD_CLASS(Foo) LLDB_FORWARD_CLASS(Bar) ... That said, my preferred solution would be something like `template<typename T> using SP = std::shared_ptr<T>`, and then replacing all `XyzSP` with `SP<Xyz>`. The two main benefits (besides simplifying the forward file) I see are: - being able to write `SP<const Xyz>`. With the current pattern, you'd have to introduce a whole new class of typedefs, or live with the fact that `shared_ptr<const xyz>` looks very different from XyzSP - being compatible with the llvm naming scheme. XyzSP and xyz_sp would collide if they both used camel case. With this, they won't because one of them will not exist. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133906/new/ https://reviews.llvm.org/D133906 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
