[Lldb-commits] [lldb] r360757 - Group forward declarations in one namespace lldb_private {}

This commit makes things look a little cleaner, but do we need any of these forward declarations at all?

Most of these files either directly include lldb-forward.h or get it from lldb-types.h or lldb-defines.h (which includes lldb-types.h). As a general practice we've been getting all the lldb_private type forward declarations this way rather than cluttering up the .h files with individual forward declarations. If we are going to keep doing it that way - which I can't see any reason not to do, then we should just delete all these forward defines, right?

Jim

A couple of perspectives FWIW:

a) Header file includes increase overall dependencies which affects
incremental rebuild time whenever you touch something. Especially more
when that header is included into multiple headers that are then
included into an array of translation units
b) Having each file contain the forward declarations it needs from the
project and no more also will help both compile time and understanding
what's used in any particular header.

As a related exercise:

https://twitter.com/echristo/status/1116609586004316160

basically has removing a couple of transitive dependencies saving in
excess of 70% of the incremental rebuild time after touching a file.
This seems significant. :slight_smile:

-eric

This is a common header file that specifies either common enums & #defines in lldb, or forward declarations of the common classes in lldb. They don't depend on any other header files in the system, they just establish the common names you should use. So the dependency analysis introduced by these files should be trivial - they really don't depend on anything else. And the only time you change them is when you add a new public type or define to lldb, which doesn't happen very often.

But it is very convenient to have a common global set of names for the public entities in lldb, that can be looked up in one place, etc. You can't actually USE any of the classes from these header files, so they don't obscure what other source files actually use - one problem with the big mongo "Cocoa.h" header file pattern. If you want to get the class definition of anything, you have to include the specific .h file for that thing. This is just a list of forward declarations. Switching to

I haven't done your experiment, but if "I depend on a header that either includes no other files or straight includes another file" really is a significant burden on the dependency analysis, then I'm not sure this pattern is actually the problem.

Jim

This is a common header file that specifies either common enums & #defines in lldb, or forward declarations of the common classes in lldb. They don't depend on any other header files in the system, they just establish the common names you should use. So the dependency analysis introduced by these files should be trivial - they really don't depend on anything else. And the only time you change them is when you add a new public type or define to lldb, which doesn't happen very often.

But it is very convenient to have a common global set of names for the public entities in lldb, that can be looked up in one place, etc. You can't actually USE any of the classes from these header files, so they don't obscure what other source files actually use - one problem with the big mongo "Cocoa.h" header file pattern. If you want to get the class definition of anything, you have to include the specific .h file for that thing. This is just a list of forward declarations. Switching to

individual forward declares uglifies the code - as these examples show.

The dependency edges caused by the file means that any time anyone
touches that file everything that depends on it is rebuilt. The way
it's included basically means you're doing a full build any time you
touch it. It's who is depending on this file or any file this file
includes, not the other way around.

If no one is expected to touch it then it's probably ok. It looks like
each of the files (lldb/include/lldb/lldb-*.h) is touched ~ once every
few weeks. Mostly I was looking at the edit-compile-debug cycle of the
person working on the patch. Means about once a month someone working
on a patch has to do a much more significant set of rebuilds than
they'd already have to do because of their patch rather than a much
more minimal set of rebuilds.

As far as having a convenient place to put public headers I do agree,
but I'd probably prefer to see that via autogenerated documentation
rather than a set of forward declarations in a single header.

Anyhow, your mileage may vary. Just giving the thoughts around the
rest of the llvm project :slight_smile:

-eric

When you add to them you are often adding some larger feature which would have required a rebuild anyway, and they go long times with no change... I have never found the rebuild required when these files are touched to be a drag on my productivity. And I really appreciate their convenience.

But thanks for your friendly advice.

Jim

(I have to confess I used this as an exercise to get started in lldb…; get some insight how files/modules are organized, etc)
I agree lldb/include/lldb/lldb-forward.h makes perfect sense to speed up edit-compile-debug cycle, but I think it is probably too large: 200+ lldb_private forward declarations plus 200+ {shared,weak}_ptr typedefs
clang/include/clang/Basic/LLVM.h and lld/include/lld/Common/LLVM.h basically serve the same purpose but they are much smaller. We can probably remove some infrequently used ones from the list.

It seems many forward declarations were added in previous refactorings. Some do seem redundant, e.g. Status (3000+ refs) Stream (1000+ refs) DataExtractor (600+ refs). We should just delete them.

If we decide to get a smaller set of lldb-forward.h forward declarations (I volunteer to do this if nobody objects…) and delete those redundant forward declarations in individual files, the remaining ones may provide some cues about the module dependencies.

Side notes: I just noticed that the cmake build system produces static archives liblldb{Base,Breakpoint,Commands,…}.a, though I configured with -DBUILD_SHARED_LIBS=on. These .a get linked into liblldb.so.9.0.0svn. We may same some cycles if these static archives are not built, but are wrapped in --start-lib liblldbfoo.o liblldbbar.o --end-lib (ELF, requires gold or lld) when linking liblldb.so.9.0.0svn.

I don't want to make a big deal out of it, but I'm also not a fan of the lldb-forward header. My two main reasons are:
- it's inconsistent with the rest of llvm, which does not have any headers of such sort (LLVM.h, which we talked about last time, is the only thing remotely similar, but that's still has a much more narrow scope)
- it makes it easier to violate layering. Eg. right now I can type something like:
void do_stuff_with(Target *);
in to a "Utility" header, and it will compile just fine because it will have the forward-declaration of the Target class available even though nothing in Utility should know about that class.

Neither of these is a big problem: this is not the most important thing we differ from llvm, and also the layering violation will become obvious once you start to implement the "do_stuff_with" function (because you will hopefully need to include the right header to get the full definition). However, for these reasons, I would prefer if we got rid of this header, or at least moved towards a world where we have one forward-declaring header for each top-level module (so "lldb/Utility/forward.h", would forward-declare only Utility stuff, etc.).

pl

When you add to them you are often adding some larger feature which would have required a rebuild anyway, and they go long times with no change... I have never found the rebuild required when these files are touched to be a drag on my productivity. And I really appreciate their convenience.
But thanks for your friendly advice.
Jim

I don't want to make a big deal out of it, but I'm also not a fan of the lldb-forward header. My two main reasons are:
- it's inconsistent with the rest of llvm, which does not have any headers of such sort (LLVM.h, which we talked about last time, is the only thing remotely similar, but that's still has a much more narrow scope)
- it makes it easier to violate layering. Eg. right now I can type something like:
void do_stuff_with(Target *);
in to a "Utility" header, and it will compile just fine because it will have the forward-declaration of the Target class available even though nothing in Utility should know about that class.

Neither of these is a big problem: this is not the most important thing we differ from llvm, and also the layering violation will become obvious once you start to implement the "do_stuff_with" function (because you will hopefully need to include the right header to get the full definition).

For sure, lldb-forward.h should only have forward declarations (or SP declarations) - as is the case now. So you can't do anything but pass an object along with only lldb-forward. I like it because it encourages not including the actual definition header files in other header files that don't need to see implementations. It provides the set of names you can refer to that lldb provides, so it is easy to make your interface declarations work w/o being tempted to grab too much. The dictate of living off lldb-forward.h actually helps to remove unnecessary dependencies. The common defines have the other benefit that if you know you should be including these you will more easily learn to look there and see LLDB_INVALID_ADDRESS rather than inventing your own local invalid address marker...

However, for these reasons, I would prefer if we got rid of this header, or at least moved towards a world where we have one forward-declaring header for each top-level module (so "lldb/Utility/forward.h", would forward-declare only Utility stuff, etc.).

I'm not opposed to the latter option, though I'm not convinced it would be worth the churn.

Jim