This is an archive of the discontinued LLVM Phabricator instance.

[LLDB] Remove cases of using namespace llvm:: from header file
ClosedPublic

Authored by shafik on Mar 2 2022, 10:54 AM.

Details

Summary

We have two cases where we do some form of using namespace llvm:: in LLDB headers. I remove those cases and modified the effected files to either use the fully qualified name of using namespace inside a function.

Diff Detail

Event Timeline

shafik created this revision.Mar 2 2022, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 10:54 AM
Herald added a subscriber: arphaman. · View Herald Transcript
shafik requested review of this revision.Mar 2 2022, 10:54 AM

I used using namespace in a few functions but I am not committed to this approach. So I am happy to hear feedback on whether we want to just use fully qualified names everywhere instead or nail down a criteria as to when it is acceptable.

We also have using namespace lldb in a few header files but I wanted to tackle that separately based on the feedback on this.

For the .cpp files with hundreds of lines modified, what do you think about adding using namespace llvm::dwarf; to those? For starters DWARFExpression.cpp, DWARFAbbreviationDeclaration.cpp, and SymbolFileDWARF.cpp.

sorry Shafik I see that you've asked that very question. I'll give a +1 to making use of using namespace within .cpp files, especially where a namespace is used pervasively. When a file uses it only a namespace infrequently, I think explicit is reasonable.

I think it's reasonable to be able to refer to the dwarf constants from within the dwarf plugin via their base names alone. The implementation -- a top-level using namespace llvm::dwarf -- is not reasonable, but that's because the plugin is very old, and completely unnamespaced.

For the newer plugins, we've started putting them in a sub-namespace of lldb_private, which means they cannot be accidentally referenced from the core code, but they can easily reference (without qualification) symbols defined in the core libraries.

If we put the dwarf plugin into a (e.g.) lldb_private::dwarf namespace, then I think it would be ok to put a using namespace llvm::dwarf into that namespace, even in a header -- because those symbols would only be visible to symbols which are in that namespace.

In other words, I'd have the dwarf plugin do what the minidump plugin is doing (disclaimer: I wrote most of that plugin).

Anyway, I'm not married to that approach, but that's what I would do...

I think it's reasonable to be able to refer to the dwarf constants from within the dwarf plugin via their base names alone. The implementation -- a top-level using namespace llvm::dwarf -- is not reasonable, but that's because the plugin is very old, and completely unnamespaced.

For the newer plugins, we've started putting them in a sub-namespace of lldb_private, which means they cannot be accidentally referenced from the core code, but they can easily reference (without qualification) symbols defined in the core libraries.

If we put the dwarf plugin into a (e.g.) lldb_private::dwarf namespace, then I think it would be ok to put a using namespace llvm::dwarf into that namespace, even in a header -- because those symbols would only be visible to symbols which are in that namespace.

In other words, I'd have the dwarf plugin do what the minidump plugin is doing (disclaimer: I wrote most of that plugin).

Anyway, I'm not married to that approach, but that's what I would do...

If I understand correctly you are proposing that I do:

namespace lldb_private {
namespace dwarf {
  using namespace llvm::dwarf;
}
}

in lldb/include/lldb/Core/dwarf.h and then in the .cpp files:

using namespace lldb_private;
using namespace dwarf;

That sounds reasonable to me.

Then I would revert the changes to the minidump files, since they are already using this strategy.

I think it's reasonable to be able to refer to the dwarf constants from within the dwarf plugin via their base names alone. The implementation -- a top-level using namespace llvm::dwarf -- is not reasonable, but that's because the plugin is very old, and completely unnamespaced.

For the newer plugins, we've started putting them in a sub-namespace of lldb_private, which means they cannot be accidentally referenced from the core code, but they can easily reference (without qualification) symbols defined in the core libraries.

If we put the dwarf plugin into a (e.g.) lldb_private::dwarf namespace, then I think it would be ok to put a using namespace llvm::dwarf into that namespace, even in a header -- because those symbols would only be visible to symbols which are in that namespace.

In other words, I'd have the dwarf plugin do what the minidump plugin is doing (disclaimer: I wrote most of that plugin).

Anyway, I'm not married to that approach, but that's what I would do...

If I understand correctly you are proposing that I do:

namespace lldb_private {
namespace dwarf {
  using namespace llvm::dwarf;
}
}

in lldb/include/lldb/Core/dwarf.h and then in the .cpp files:

using namespace lldb_private;
using namespace dwarf;

Yes, that's the general idea. I'm not entirely sure how to apply that to the DWARFExpression class, since it is not really a part of the dwarf plugin. But all of the uses of dwarf constants are in the cpp file, so I suppose a using namespace llvm::dwarf in DWARFExpression.cpp file should suffice.

shafik updated this revision to Diff 412548.Mar 2 2022, 2:41 PM

Updating diff based on comments

JDevlieghere added inline comments.Mar 2 2022, 3:59 PM
lldb/source/Expression/DWARFExpression.cpp
46

Why not lldb_private::dwarf?

lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
19

Wouldn't it be more consistent to use lldb_private::dwarf everywhere?

JDevlieghere added inline comments.Mar 2 2022, 4:01 PM
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
34
shafik added inline comments.Mar 2 2022, 7:04 PM
lldb/source/Expression/DWARFExpression.cpp
46

I think I started out with this file and must have written that by mistake.

lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
19

Yeah, I was trying to match the style of how it was done for minidump, see source/Plugins/Process/minidump/MinidumpParser.cpp.

Since we need access to both namespaces but in files that did already have using namespace lldb_private; I just used the less verbose version.

I can see arguments for just being verbose everywhere.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
34

Yeah I noticed that was going to put that on a list of things to do, didn't feel right mixing that in here as well.

JDevlieghere added inline comments.Mar 2 2022, 11:07 PM
lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
19

Maybe it's just preference? Curious to hear from @labath why he did it that way. Personally I prefer the slightly more verbose one because it's self contained.

lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
34

Totally!

labath added inline comments.Mar 3 2022, 1:48 AM
lldb/source/Plugins/SymbolFile/DWARF/AppleDWARFIndex.cpp
19

Actually, I totally agree with you here. I think I did not personally write that using directive. I recall noticing it (during review, or during subsequent edits), but not considering it worth the trouble of changing it.

So +1 to verbose using directives.

labath accepted this revision.Mar 3 2022, 1:49 AM
This revision is now accepted and ready to land.Mar 3 2022, 1:49 AM
shafik updated this revision to Diff 412751.Mar 3 2022, 9:52 AM
shafik marked 6 inline comments as done.

Moved to using namespace lldb_private::dwarf everywhere since the consensus is that being verbose is preferred.

Herald added a project: Restricted Project. · View Herald TranscriptMar 3 2022, 10:42 AM