[Lldb-commits] [PATCH] D105215: [lldb] Remove CPlusPlusLanguage from Mangled

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

My comments were mostly about how we should refactor in a perfect world. Not sure we need to solve that right away. I am ok with this patch as it is if Jim is.

Jim? What do you think about this patch? OK? I am good if you are.



================
Comment at: lldb/source/Core/Mangled.cpp:322
   if (preference == ePreferDemangledWithoutArguments) {
-    return GetDemangledNameWithoutArguments(m_mangled, demangled);
+    if (Language *lang = Language::FindPlugin(GuessLanguage())) {
+      return lang->GetDemangledFunctionNameWithoutArguments(*this);
----------------
bulbazord wrote:
> clayborg wrote:
> > Maybe we should make a Language::FindPlugin(...) that like:
> > ```
> > Language *Language::FindPlugin(Mangled::ManglingScheme mangling_scheme);
> > ```
> > Should be easy to add since this change is kind of about refactoring and putting the code into plug-ins. It is essentially what "lldb::LanguageType Mangled::GuessLanguage() const" is doing. That code could be moved to where Language::FindPlugin(...) lives.
> `GuessLanguage` has a bunch of uses outside of `Mangled` itself, so I think that would make more sense if we performed other refactors first.
> 
> I don't think putting this into another `Language::FindPlugin` function is a good idea because (as I understand it) mangling schemes aren't specific to languages. 
We _are_ using GuessLanguage to guess the language and we are using a language specific plug-in to do the work. So effectively it is the same thing.

I wonder if GetName(...) should take an extra Language parameter? If a lldb_private::Function wants the name as , then we _do_ know the language, then it could send the language in for it to be more correct. Itanium name mangling can mangle different languages right? Is there any way to differentiate correctly without actually knowing the language?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105215