[Lldb-commits] [PATCH] D60817: [NativePDB] Add anonymous namespaces support

Adrian McCarthy via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Apr 18 13:35:53 PDT 2019

amccarth added a comment.

Sorry for the slow response; I'm still learning about this code.

I like where this is going.

Comment at: lit/SymbolFile/NativePDB/ast-types.cpp:77
+// FIXME: Should be located in the namespace `A`, in the struct `C<1>`.
+A::C<1>::D AC1D;
Is this a new problem because of your change?  Or is this an already existing problem you discovered in the process?  Is the cause understood?  Should you add a commented-out CHECK showing what the test would be when this problem is fixed?

Comment at: source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:575
     std::string ns = name_components.front()->toString();
-    context = m_clang.GetUniqueNamespaceDeclaration(ns.c_str(), context);
+    context = GetOrCreateNamespaceDecl(ns.c_str(), *context);
     name_components = name_components.drop_front();
Again, I think you should drop the `.c_str()`.

Comment at: source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:812
 clang::NamespaceDecl *
 PdbAstBuilder::GetOrCreateNamespaceDecl(llvm::StringRef name,
                                         clang::DeclContext &context) {
Normally, I would agree that `StringRef` is the right type for `name`, but looking at the three call sites, it seems that this is going to cause a lot of unnecessary conversions.

The call sites are all passing std::string::c_str(), which is a `const char *`.  So we're constructing a `StringRef` `name` (which involves a `strlen`).  The `name` is then passed on as `name.str().c_str()` which constructs a new `std::string` only to pass a `const char *` again.

I'd argue that, in this case, we're better off declaring `name` as a `const char *`.

Comment at: source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:813
 PdbAstBuilder::GetOrCreateNamespaceDecl(llvm::StringRef name,
                                         clang::DeclContext &context) {
+  return m_clang.GetUniqueNamespaceDeclaration(
I'd also recommend making `GetOrCreateNamespaceDecl` private, since the only callers are here in `PdbAstBuilder.cpp`.




More information about the lldb-commits mailing list