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

[Sean Callanan via lldb-commits]

spyffe requested changes to this revision.
spyffe added a comment.
This revision now requires changes to proceed.

Siva, this is a clever and self-contained solution to a problem that's annoyed us a great deal for a while now.  I have a few minor quibbles about implementation but overall this is a great fix!  Thank you!


================
Comment at: source/Expression/ExpressionSourceCode.cpp:306
@@ +305,3 @@
+        ConstString object_name;
+        if (IsCppMethod(frame, object_name))
+        {
----------------
I think we should do this in all C++ cases.  The reason is because it'll make sure the method gets more testing and if anything fails we'll catch it in the common case instead of seeing it only in specific "edge" cases.  Could we just check if the frame is C++?

================
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)
----------------
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.


http://reviews.llvm.org/D16746