sivachandra marked an inline comment as done. sivachandra added a comment. Thanks, Greg and Sean for the review. The new version of the patch has tests also added. I also have an inline response to a comment by Sean.
================ Comment at: source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:1381 @@ +1380,3 @@ + clang::NamespaceDecl *namespace_decl = ast->GetUniqueNamespaceDeclaration( + name_unique_cstr, nullptr, m_ast_context); + if (namespace_decl) ---------------- spyffe wrote: > This bit where we're passing the AST context as an argument as well as the > this pointer is a bit awkward. The common pattern when we have a class (like > lldb::ClangASTContext) that provides useful functionality on top of another > class (clang::ASTContext, in this case) is to add a method: > ``` > static void GetUniqueNamespaceDeclaration(clang::ASTContext *, …) > ``` > and then have the instance method call through to it transparently. Unless > there's some reason why the frame's AST context is important, let's just do > that here too. I did not understand this comment fully. However, the frame's AST is important here because we want the new namespace decl to have the same clang::ExternalASTSource as that of the current frame. The ClangASTContext object |ast| above does not come with any AST set, and hence GetUniqueNamespaceDeclaration makes one up with external AST source set to ClangExternalASTSourceCallbacks instead of clang_internal::ClangASTSource. To avoid this, I added the new argument which specifies the AST to be used. http://reviews.llvm.org/D16746 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
