[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D108229#2966405 , @aprantl wrote:

> FYI (you may have have already noticed) this causes some merge conflicts in 
> swift-lldb. It would be great if we could work together to figure out how to 
> best resolve them.

Yes, I figured that would happen at some point. I have an interest in seeing 
swift-lldb apply this change (and some of my other refactoring patches) so I 
would love to work with you and anyone else to get this change in smoothly. 
Unfortunately I had to revert this patch. It appears my environment had some 
issues that prevented me from running about half of lldb's test suite so I 
didn't catch any of the failures. I was finally able to debug the failing tests 
today and I realized that this change has several issues that need to be 
addressed. Once I have time to work through those issues and resubmit this 
patch, I'll add you as a reviewer and we can go from there. Does this sound 
good to you?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108229/new/

https://reviews.llvm.org/D108229

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

FYI (you may have have already noticed) this causes some merge conflicts in 
swift-lldb. It would be great if we could work together to figure out how to 
best resolve them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108229/new/

https://reviews.llvm.org/D108229

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Alex Langford via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcd2134e42aa7: [lldb] Refactor Module::LookupInfo constructor 
(authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108229/new/

https://reviews.llvm.org/D108229

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Target/Language.h
  lldb/source/Core/CMakeLists.txt
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
@@ -104,6 +104,9 @@
   std::vector
   GetMethodNameVariants(ConstString method_name) const override;
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   lldb::TypeCategoryImplSP GetFormatters() override;
Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -275,6 +275,22 @@
   return variant_names;
 }
 
+Language::FunctionNameInfo
+ObjCLanguage::GetFunctionNameInfo(ConstString name) const {
+  Language::FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (ObjCLanguage::IsPossibleObjCMethodName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  if (ObjCLanguage::IsPossibleObjCSelector(name.GetCString())) {
+info.func_name_type |= lldb::eFunctionNameTypeSelector;
+  }
+
+  return info;
+}
+
 bool ObjCLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   ConstString demangled_name = mangled.GetDemangledName();
   if (!demangled_name)
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -104,6 +104,9 @@
 
   static lldb_private::ConstString GetPluginNameStatic();
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   ConstString
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -59,6 +59,39 @@
   return g_name;
 }
 
+Language::FunctionNameInfo
+CPlusPlusLanguage::GetFunctionNameInfo(ConstString name) const {
+  FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (IsCPPMangledName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  CPlusPlusLanguage::MethodName method(name);
+  llvm::StringRef basename = method.GetBasename();
+  if (basename.empty()) {
+if (CPlusPlusLanguage::ExtractContextAndIdentifier(
+name.GetCString(), info.context, info.basename)) {
+  info.func_name_type |=
+  (lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+} else {
+  info.func_name_type |= lldb::eFunctionNameTypeFull;
+}
+  } else {
+info.func_name_type |=
+(lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+  }
+
+  if (!method.GetQualifiers().empty()) {
+// There is a 'const' or other qualifier following the end of the function
+// parens, this can't be a eFunctionNameTypeBase.
+info.func_name_type &= ~(lldb::eFunctionNameTypeBase);
+  }
+
+  return info;
+}
+
 bool CPlusPlusLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   const char *mangled_name = mangled.GetMangledName().GetCString();
   return mangled_name && CPlusPlusLanguage::IsCPPMangledName(mangled_name);
Index: lldb/source/Core/Module.cpp
===
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -51,7 +51,6 @@
 #endif
 
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
-#include "Plugins/Language/ObjC/ObjCLanguage.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Compiler.h"
@@ -635,101 +634,81 @@
 
 Module::LookupInfo::LookupInfo(ConstString name,
FunctionNameType name_type_mask,
-   LanguageType language)
-: m_name(name), m_lookup_name(), m_language(language),
+   

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:73
+  llvm::StringRef basename = method.GetBasename();
+  if (basename.empty()) {
+if (CPlusPlusLanguage::ExtractContextAndIdentifier(

jingham wrote:
> The old code checked method.IsValid before pulling out the base name.  It 
> seems sensible that an invalid MethodName would return an empty base name, 
> but that does seem to rely a little on implicit behavior?  Not sure that's a 
> big deal.
Yep, this does rely on implicit behavior. I don't think it's a huge deal, but 
it's easily changeable. :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108229/new/

https://reviews.llvm.org/D108229

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.
This revision is now accepted and ready to land.

I like the change in general, anywhere that we're explicitly listing languages 
in generic code we're probably doing it wrong...  So far as I can tell you've 
reconstructed the old behaviors correctly in the new setup.  I left one trivial 
comment, but really this LGTM.




Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:73
+  llvm::StringRef basename = method.GetBasename();
+  if (basename.empty()) {
+if (CPlusPlusLanguage::ExtractContextAndIdentifier(

The old code checked method.IsValid before pulling out the base name.  It seems 
sensible that an invalid MethodName would return an empty base name, but that 
does seem to rely a little on implicit behavior?  Not sure that's a big deal.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108229/new/

https://reviews.llvm.org/D108229

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 367244.
bulbazord added a comment.

Delete now unused variable


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108229/new/

https://reviews.llvm.org/D108229

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Target/Language.h
  lldb/source/Core/CMakeLists.txt
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
@@ -104,6 +104,9 @@
   std::vector
   GetMethodNameVariants(ConstString method_name) const override;
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   lldb::TypeCategoryImplSP GetFormatters() override;
Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -275,6 +275,22 @@
   return variant_names;
 }
 
+Language::FunctionNameInfo
+ObjCLanguage::GetFunctionNameInfo(ConstString name) const {
+  Language::FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (ObjCLanguage::IsPossibleObjCMethodName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  if (ObjCLanguage::IsPossibleObjCSelector(name.GetCString())) {
+info.func_name_type |= lldb::eFunctionNameTypeSelector;
+  }
+
+  return info;
+}
+
 bool ObjCLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   ConstString demangled_name = mangled.GetDemangledName();
   if (!demangled_name)
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -104,6 +104,9 @@
 
   static lldb_private::ConstString GetPluginNameStatic();
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   ConstString
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -59,6 +59,39 @@
   return g_name;
 }
 
+Language::FunctionNameInfo
+CPlusPlusLanguage::GetFunctionNameInfo(ConstString name) const {
+  FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (IsCPPMangledName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  CPlusPlusLanguage::MethodName method(name);
+  llvm::StringRef basename = method.GetBasename();
+  if (basename.empty()) {
+if (CPlusPlusLanguage::ExtractContextAndIdentifier(
+name.GetCString(), info.context, info.basename)) {
+  info.func_name_type |=
+  (lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+} else {
+  info.func_name_type |= lldb::eFunctionNameTypeFull;
+}
+  } else {
+info.func_name_type |=
+(lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+  }
+
+  if (!method.GetQualifiers().empty()) {
+// There is a 'const' or other qualifier following the end of the function
+// parens, this can't be a eFunctionNameTypeBase.
+info.func_name_type &= ~(lldb::eFunctionNameTypeBase);
+  }
+
+  return info;
+}
+
 bool CPlusPlusLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   const char *mangled_name = mangled.GetMangledName().GetCString();
   return mangled_name && CPlusPlusLanguage::IsCPPMangledName(mangled_name);
Index: lldb/source/Core/Module.cpp
===
--- lldb/source/Core/Module.cpp
+++ lldb/source/Core/Module.cpp
@@ -51,7 +51,6 @@
 #endif
 
 #include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h"
-#include "Plugins/Language/ObjC/ObjCLanguage.h"
 
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Compiler.h"
@@ -635,101 +634,81 @@
 
 Module::LookupInfo::LookupInfo(ConstString name,
FunctionNameType name_type_mask,
-   LanguageType language)
-: m_name(name), m_lookup_name(), m_language(language),
+   LanguageType language_type)
+: m_name(name), m_lookup_name(name), m_language_type(language_type),
   

[Lldb-commits] [PATCH] D108229: [lldb] Refactor Module::LookupInfo constructor

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: clayborg, jingham, teemperor.
Herald added a subscriber: mgorny.
bulbazord requested review of this revision.
Herald added a project: LLDB.

Module::LookupInfo's constructor currently goes over supported languages
trying to figure out the best way to search for a symbol name. This
seems like a great candidate for refactoring. Specifically, this is work
that can be delegated to language plugins.

Once again, the goal here is to further decouple plugins from
non-plugins. The idea is to have each language plugin take a name and
give you back some information about the name from the perspective of
the language. Specifically, each language now implements a
`GetFunctionNameInfo` method which returns an object of type
`Language::FunctionNameInfo`. Right now, it consists of a basename,
a context, and a FunctionNameType. Module::LookupInfo's constructor will
call `GetFunctionNameInfo` with the appropriate language plugin(s) and
then decide what to do with that information. I have attempted to maintain
existing behavior as best as possible.

A nice side effect of this change is that lldbCore no longer links
against the ObjC Language plugin.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108229

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Target/Language.h
  lldb/source/Core/CMakeLists.txt
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
  lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
  lldb/source/Plugins/Language/ObjC/ObjCLanguage.h

Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.h
@@ -104,6 +104,9 @@
   std::vector
   GetMethodNameVariants(ConstString method_name) const override;
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   lldb::TypeCategoryImplSP GetFormatters() override;
Index: lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
===
--- lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
+++ lldb/source/Plugins/Language/ObjC/ObjCLanguage.cpp
@@ -275,6 +275,22 @@
   return variant_names;
 }
 
+Language::FunctionNameInfo
+ObjCLanguage::GetFunctionNameInfo(ConstString name) const {
+  Language::FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (ObjCLanguage::IsPossibleObjCMethodName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  if (ObjCLanguage::IsPossibleObjCSelector(name.GetCString())) {
+info.func_name_type |= lldb::eFunctionNameTypeSelector;
+  }
+
+  return info;
+}
+
 bool ObjCLanguage::SymbolNameFitsToLanguage(Mangled mangled) const {
   ConstString demangled_name = mangled.GetDemangledName();
   if (!demangled_name)
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.h
@@ -104,6 +104,9 @@
 
   static lldb_private::ConstString GetPluginNameStatic();
 
+  Language::FunctionNameInfo
+  GetFunctionNameInfo(ConstString name) const override;
+
   bool SymbolNameFitsToLanguage(Mangled mangled) const override;
 
   ConstString
Index: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
===
--- lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
+++ lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp
@@ -59,6 +59,39 @@
   return g_name;
 }
 
+Language::FunctionNameInfo
+CPlusPlusLanguage::GetFunctionNameInfo(ConstString name) const {
+  FunctionNameInfo info;
+  info.func_name_type = lldb::eFunctionNameTypeNone;
+
+  if (IsCPPMangledName(name.GetCString())) {
+info.func_name_type = lldb::eFunctionNameTypeFull;
+  }
+
+  CPlusPlusLanguage::MethodName method(name);
+  llvm::StringRef basename = method.GetBasename();
+  if (basename.empty()) {
+if (CPlusPlusLanguage::ExtractContextAndIdentifier(
+name.GetCString(), info.context, info.basename)) {
+  info.func_name_type |=
+  (lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+} else {
+  info.func_name_type |= lldb::eFunctionNameTypeFull;
+}
+  } else {
+info.func_name_type |=
+(lldb::eFunctionNameTypeMethod | eFunctionNameTypeBase);
+  }
+
+  if (!method.GetQualifiers().empty()) {
+// There is a 'const' or other qualifier following the end of the function
+// parens, this can't be a eFunctionNameTypeBase.
+info.func_name_type &= ~(lldb::eFunctionNameTypeBase);
+  }
+