JDevlieghere added a comment. In D133906#3792230 <https://reviews.llvm.org/D133906#3792230>, @labath wrote:
> 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) > ... Works for me, but I don't see how that would help with go-to definition. Xcode still won't show you the macro expansion so there's nothing to click through, which was Jim's complaint. > 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. This would address Jim's concern, but it's more churn. If everyone's okay with this approach I'm happy to do the mechanical find-and-replace. 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
