[Lldb-commits] [PATCH] D16746: Use an artifical namespace so that member vars do not hide local vars.

[Siva Chandra via lldb-commits]

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