[RFC] Adding a clang-style LLVM.h (or, "Are you tired of typing 'llvm::' everywhere ?")

Hello all,

some llvm classes, are so well-known and widely used, that qualifying them with "llvm::" serves no useful purpose and only adds visual noise. I'm thinking here mainly of ADT classes like String/ArrayRef, Optional/Error, etc. I propose we stop explicitly qualifying these classes.

We can implement this proposal the same way as clang solved the same problem, which is by creating a special LLVM.h <https://github.com/llvm-mirror/clang/blob/master/include/clang/Basic/LLVM.h> header in the Utility library. This header would adopt these classes into the lldb_private namespace via a series of forward and "using" declarations.

I think clang's LLVM.h is contains a well-balanced collection of adopted classes, and it should cover the most widely-used classes in lldb too, so I propose we use that as a starting point.

What do you think?

regards,
pavel

PS: I'm not proposing any wholesale removal of "llvm::" qualifiers from these types, though I may do some smaller-scale removals if I'm about to substantially modify a file.

Hey Pavel,

Sounds like a good idea. I don’t have a strong opinion on this matter, but I’m always in favor of improving readability.

Cheers,
Jonas

I don’t have a strong opinion, but I lean against the idea for two reasons:

  1. The llvm:: prefixes don’t really hinder readability for me. They’re like std:: prefixes on all the C++ standard library types, which I’m perfectly happy to type and read–moreso than using declarations. Sure, anybody who’s been here a while knows which classes come from LLVM, but new folks might build that knowledge by seeing the prefixes.

  2. I’m not a fan of forward declaring types provided by other parts of the code, as it requires intimate knowledge of implementation details. In practice this may not matter much for the types we’re considering. If it grew more widespread, however, I’d be more concerned. (Somewhere I’ve written a long explanation of this opinion. I’ll go search for it if anyone cares. The Google style guide discourages forward declarations, but the rationale given there isn’t as persuasive.)

Thanks for the replies. I was hoping to get more positive feedback for this, so given the current mixed-feelings replies, I think I'll just give up on this idea, unless a more vocal supporter appears (probably not the best idea to send this out just before the easter holidays).

In the mean time, here are my thoughts on what was said.

I don't have a strong opinion, but I lean against the idea for two reasons:

1. The `llvm::` prefixes don't really hinder readability for me. They're like `std::` prefixes on all the C++ standard library types, which I'm perfectly happy to type and read--moreso than using declarations. Sure, anybody who's been here a while knows which classes come from LLVM, but new folks might build that knowledge by seeing the prefixes.

Yeah, I was wondering why I'm bothered by typing "llvm::" and not by "std::". I concluded that this is down to two things:
1. we don't use that many things from the std:: namespace actually. Pretty much everything except std::string and std::vector is discouraged because llvm has better alternatives

2. llvm names are longer. This is not just due to to "llvm" prefix, which is just one char, but also the class names themselves tend to be longer. std::vector vs llvm::SmallVector, std::map vs. llvm::DenseMap, std::string vs. llvm::StringRef, etc.

This effect gets multiplied once you start to combine things. For instance if you have a function returning Expected<ArrayRef<T>> (which is not an unreasonable thing to do), then by the time you spell out the full type, more than half of your usable horizontal space is gone. Because of this, I've found myself using "auto" or relying on ADL more and more often, which I don't consider very ideal either.

I don't think using "auto" is always a good choice because it hides interesting details. E.g. an Optional<T> can look a lot like Expected<T>, but there are differences in how they are supposed used which should not be overlooked (I wish I was able to type "Expected<auto>" :P). And ADL is sometimes just too magical...

2. I'm not a fan of forward declaring types provided by other parts of the code, as it requires intimate knowledge of implementation details. In practice this may not matter much for the types we're considering. If it grew more widespread, however, I'd be more concerned. (Somewhere I've written a long explanation of this opinion. I'll go search for it if anyone cares. The Google style guide discourages forward declarations, but the rationale given there isn't as persuasive.)

Yeah, I agree the forward declarations are not ideal (and the clang file did raise my eyebrows when I first saw it), but after a while I started to like it.

FWIW, I wouldn't be opposed to just #including the relevant files instead of forward-declaring stuff, but I think doing it the same way is better for consistency.

Out of interest, I took a look at what lld is doing. I've found that while it doesn't have a LLVM.h equivalent, it is a heavy user of "using namespace llvm" (about 2 out of 3 cpp files have it). This approach wouldn't work that well for us because of naming conflicts ("Module"), and I would consider it inferior for the same reason that "using namespace std" is discouraged -- it just brings in too much stuff into your scope.

regards,
pl

I’d have no objection to individual .cpp files having a few using declarations for the specific types that file cares about:

#include “llvm/ADT/ArrayRef.h”

#include “llvm/ADT/Optional.h”

using llvm::ArrayRef;
using llvm::Optional;

And then the rest of the file uses the unqualified ArrayRef and Optional. If it’s just a few commonly used types, this get the improvements in typability and readability that you’re looking for.

But keeps the aliasing local to the implementation file, which reduces the risks of conflicts and helps newbies understand where these types come from.

It also avoids the headaches of forward declarations.

LLDB has only a couple using declarations like this right now, but other parts of LLVM seem to do this more liberally.

LLD has include/lld/Common/LLVM.h, which is included in 60 or so places.

Woops. That's actually a pretty embarrassing mistake. Thanks for correcting me. :slight_smile:

some llvm classes, are so well-known and widely used, that qualifying them
with "llvm::" serves no useful purpose and only adds visual noise. I'm
thinking here mainly of ADT classes like String/ArrayRef, Optional/Error,
etc. I propose we stop explicitly qualifying these classes.

...

What do you think?

If I should say something I would keep llvm::.

My reason: The LLVM types are in many cases emulating classes adopted
in future C++ standards and I find more clear llvm:: vs. std:: than
"" vs. std::. Moreover when std:: is commonly omitted in other projects.

Jan