clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Just a few cleanups to do mentioned in the inlined comments.
================
Comment at: include/lldb/Core/Module.h:950
@@ -949,1 +949,3 @@
+ GoASTContext &GetGoASTContext();
+
----------------
We should remove this and just use Module::GetTypeSystemForLanguage(...) to get
at this. Then from the type system you can say
"type_system->GetAsGoASTContext()". ClangASTContext has been special because we
started out thinking this would be the one and only type system we would ever
use in the debugger and the "Module::GetClangASTContext()" should be removed. I
will do that in a future patch, but for now,remove this function and use
"Module::GetTypeSystemForLanguage(eLanguageTypeGo)" when you need it.
================
Comment at: include/lldb/Symbol/ClangASTContext.h:489-493
@@ +488,7 @@
+
+ GoASTContext *
+ AsGoASTContext() override
+ {
+ return nullptr;
+ }
+
----------------
Add "TypeSystem::AsXXXASTContext()" functions should have a default
implementation in the TypeSystem base class which returns nullptr so every
other TypeSystem doesn't have to implement the function and return nullptr.
================
Comment at: include/lldb/Symbol/CompilerType.h:21
@@ +20,3 @@
+{
+class Type;
+}
----------------
indent one level
================
Comment at: include/lldb/lldb-forward.h:102
@@ -101,2 +101,3 @@
class Flags;
+class GoASTContext;
class TypeCategoryImpl;
----------------
there must be a tab here? We use spaces in LLDB, so please space out to match
other indentation.
================
Comment at: source/Core/Module.cpp:27
@@ -26,2 +26,3 @@
#include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Symbol/GoASTContext.h"
#include "lldb/Symbol/CompileUnit.h"
----------------
Remove this duplicate include and use the one below.
================
Comment at: source/Core/Module.cpp:152
@@ -149,2 +151,3 @@
m_ast (new ClangASTContext),
+ m_go_ast(new GoASTContext),
m_source_mappings (),
----------------
Default construct so it is null until a call to
Module::GetTypeSystemForLanguage() with eLanguageTypeGo is passed in. We can
eventually make the TypeSystem objects into plug-ins, but for now we can hard
code this. We are going to remove "GoASTContext &Module::GetGoASTContext()" in
favor of using Module::GetTypeSystemForLanguage(eLanguageTypeGo) so we don't
need to construct one as we don't need to return an reference.
================
Comment at: source/Core/Module.cpp:258
@@ -253,2 +257,3 @@
m_ast (new ClangASTContext),
+ m_go_ast(new GoASTContext),
m_source_mappings (),
----------------
Default construct so it is null until a call to
Module::GetTypeSystemForLanguage() with eLanguageTypeGo is passed in. We can
eventually make the TypeSystem objects into plug-ins, but for now we can hard
code this. We are going to remove "GoASTContext &Module::GetGoASTContext()" in
favor of using Module::GetTypeSystemForLanguage(eLanguageTypeGo) so we don't
need to construct one as we don't need to return an reference.
================
Comment at: source/Core/Module.cpp:306
@@ -299,2 +305,3 @@
m_ast (new ClangASTContext),
+ m_go_ast(new GoASTContext),
m_source_mappings (),
----------------
Default construct so it is null until a call to
Module::GetTypeSystemForLanguage() with eLanguageTypeGo is passed in. We can
eventually make the TypeSystem objects into plug-ins, but for now we can hard
code this. We are going to remove "GoASTContext &Module::GetGoASTContext()" in
favor of using Module::GetTypeSystemForLanguage(eLanguageTypeGo) so we don't
need to construct one as we don't need to return an reference.
================
Comment at: source/Core/Module.cpp:432
@@ +431,3 @@
+ {
+ return &GetGoASTContext();
+ }
----------------
This code will now check for m_go_ast and if it is NULL, then it will construct
a GoASTContext and place it into m_go_ast. This allows us to create
GoASTContext on demand.
================
Comment at: source/Core/Module.cpp:480-495
@@ +479,18 @@
+
+GoASTContext &
+Module::GetGoASTContext ()
+{
+ Mutex::Locker locker (m_mutex);
+ if (m_did_init_go == false)
+ {
+ ObjectFile * objfile = GetObjectFile();
+ ArchSpec object_arch;
+ if (objfile && objfile->GetArchitecture(object_arch))
+ {
+ m_did_init_go = true;
+ m_go_ast->SetAddressByteSize(object_arch.GetAddressByteSize());
+ }
+ }
+ return *m_go_ast;
+}
+
----------------
Remove this and move initialization into Module::GetTypeSystemForLanguage()
inside the Go language if clause. We probably don't need the m_did_init_go
member variable anymore since you can key off of the m_go_ast being NULL to
know that you need to init this.
================
Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:513-514
@@ +512,4 @@
+ type_system =
m_obj_file->GetModule()->GetTypeSystemForLanguage(language);
+ if (type_system)
+ type_system->SetSymbolFile(this);
+ return type_system;
----------------
These two lines can be moved into the else clause since we only need to
manually set the type system if we actually get the type system from the
module. So this code should be:
```
SymbolFileDWARFDebugMap * debug_map_symfile = GetDebugMapSymfile ();
TypeSystem *type_system;
if (debug_map_symfile)
type_system = debug_map_symfile->GetTypeSystemForLanguage(language);
else
{
type_system =
m_obj_file->GetModule()->GetTypeSystemForLanguage(language);
if (type_system)
type_system->SetSymbolFile(this);
}
```
================
Comment at: source/Symbol/ClangASTContext.cpp:3042
@@ -3041,3 +3041,3 @@
{
- if (type)
+ if (type && type.GetTypeSystem()->AsClangASTContext())
return GetCanonicalQualType(type)->isObjCObjectOrInterfaceType();
----------------
Feel free to add a function like:
```
bool
CompilerType::IsClangType ();
```
So we don't have to have code like above:
```
if (type && type.GetTypeSystem()->AsClangASTContext())
```
And we can just change all of these to:
```
if (type.IsClangType())
```
Repository:
rL LLVM
http://reviews.llvm.org/D12585
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
