[Lldb-commits] [PATCH] D12585: Add a TypeSystem for Go

[Greg Clayton via lldb-commits]

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