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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
