This is an archive of the discontinued LLVM Phabricator instance.

[lldb] Generate lldb-forward with .def file
AcceptedPublic

Authored by JDevlieghere on Sep 14 2022, 4:05 PM.

Details

Summary

Generate lldb-forward with .def file. The benefit is that we don't have to keep the list of classes, and typedefs in sync.

Diff Detail

Event Timeline

JDevlieghere created this revision.Sep 14 2022, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2022, 4:05 PM
JDevlieghere requested review of this revision.Sep 14 2022, 4:05 PM
aprantl accepted this revision.Sep 14 2022, 4:07 PM

Definitely better than the previous contents!

This revision is now accepted and ready to land.Sep 14 2022, 4:07 PM
jingham added a subscriber: jingham.EditedSep 14 2022, 4:43 PM

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.

mib added a comment.Sep 14 2022, 5:36 PM
This comment was removed by mib.
mib accepted this revision.EditedSep 14 2022, 5:37 PM

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.

@jingham to be honest, as a fellow Xcode user, this indirection annoyed me more than I found it convenient. May be we should have Xcode go directly to the template argument class when jumping to definition on smart pointers.

@JDevlieghere LGTM! Thanks!

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.

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.

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.

SP and UP templates seems fine to me.

To some extent, the fact that we define an XxxSP for a class is saying "This is a thing we mostly pass around as a shared pointer" which would be lost if every time we make an SP or UP we use the same syntax. But I'm not sure that that's a terribly strong objection.

I really wish the .def files would actually generate a header file in the output directory. I am not a fan of using code navigation when saying "take me to the definition of "class Block;" and it shows:

#define LLDB_FORWARD_CLASS(Name) class Name;
#include "lldb-forward.def"
#undef LLDB_FORWARD_CLASS

Also as Jim pointed out, we now will generate shared, unique, and weak pointers for everything, even if those classes are never used that way.

I am not a fan of this. Wouldn't this also slow down compilation a bit? Each file that #include "lldb-forward.h" will not do a large amount of preprocessor stuff for each and every file that is compiled that includes this?

// 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.

Right, I see. I misunderstood the problem somehow. That said, I can imagine Xcode offering a (clickable) tooltip showin the macro expansion in this case. I know of an IDE that does that, and it's pretty cool.

Wouldn't this also slow down compilation a bit? Each file that #include "lldb-forward.h" will not do a large amount of preprocessor stuff for each and every file that is compiled that includes this?

Technically yes, but I very much doubt the difference will be measurable. The slowness of compiling c++ comes from instantiating templates and other fancy stuff. The preprocessor step is ridiculously fast.