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

[Adrian McCarthy via Phabricator via lldb-commits]

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


Repository:
  rLLDB LLDB

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60817/new/

https://reviews.llvm.org/D60817