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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
