Page MenuHomePhabricator

[Symbol] Add TypeSystem::GetClassName
ClosedPublic

Authored by xiaobai on Nov 4 2019, 12:57 PM.

Details

Summary

This will generalize some behavior in ValueObject. Although this code
path is still specific to C++, it no longer references ClangASTContext directly.
This will allow us to move ClangASTContext out of Symbol in the near future.

Event Timeline

xiaobai created this revision.Nov 4 2019, 12:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2019, 12:57 PM
shafik added a subscriber: shafik.Nov 4 2019, 1:48 PM

As far as I can tell we don't have coverage for GetBaseClassPath it would be good to add it while we are refactoring this code.

Some minor comments to make this more NFC.

lldb/source/Core/ValueObject.cpp
2042

Can't you just call GetCompilerType().GetTypeSystem()? This way this whole code isn't C++ specific and much shorter. It also fixes that we end up in the Scratch type system for a language when the CompilerType might not even be from that TypeSystem (but from another TypeSystem for the same language).

lldb/source/Symbol/ClangASTContext.cpp
3546

ClangUtil::IsClangType(compiler_type) seems like a more complete check. Then you can also drop the lang_type parameter.

xiaobai updated this revision to Diff 232960.Dec 9 2019, 4:39 PM

Address feedback from @teemperor

jingham added a subscriber: jingham.Dec 9 2019, 4:51 PM

One comment on a part you didn't change, but left a C++ specific track in the generic ValueObject code.

lldb/source/Core/ValueObject.cpp
2056

That's still implicitly C. Other languages (swift) use "." for the same purposes. If this is really language agnostic we either need a "GetSeparator" function, or we need s "ComposePath" operation from the language.

xiaobai added inline comments.Dec 9 2019, 5:01 PM
lldb/source/Core/ValueObject.cpp
2056

Yep, it is. We would need to get the language plugin for whatever we're dealing with. There might be a decent way to get to that by getting the LanguageType from the CompilerType, and trying to find a plugin for that (and some default separator for languages that don't have a plugin). Definitely worth doing.

I think adding a "GetClassSeparator" method in this patch would stray from the purpose of this patch, so I'd like to do that in a follow-up if possible.

jingham added inline comments.Dec 9 2019, 5:13 PM
lldb/source/Core/ValueObject.cpp
2056

FBM, but can you at least leave a FIXME here so if you don't get to the follow-up right away it's clear this still needs to be done?

xiaobai updated this revision to Diff 233163.EditedDec 10 2019, 11:37 AM

Add fixme

This revision is now accepted and ready to land.Jan 10 2020, 3:39 PM

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

lldb/include/lldb/Symbol/TypeSystem.h
184–185

Maybe add an extra argument to get a fully qualified class name?

/// \param fully_qualified Get the the class basename if false, or a fully qualified name if true
virtual llvm::Optional<std::string>
  GetClassName(const CompilerType &compiler_type, bool fully_qualified) = 0;

Then the code in the dumper doesn't need to use "::" for all languages in ValueObject::GetBaseClassPath

lldb/source/Core/ValueObject.cpp
2051–2061
bool fully_qualified = true;
llvm::Optional<std::string> class_name = type_system->GetClassName(GetCompilerType(), fully_qualified);
if (class_name)
  s.PutCString(class_name.getValue());

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

I think that you would have to add more than a fully_qualified argument to the method. You need to reference the parent ValueObject to fully qualify something as far as I can tell, so you'd have to pass along the parent as well. I think that it sounds nice for the TypeSystem to fully qualify the class name but I don't think that it's necessarily better than what exists now. You'll still have recursion walking the parent ValueObjects, you've just split it across two different methods in different classes. Maybe I'm overthinking this or missing something though?

Or do we

Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?

I think that you would have to add more than a fully_qualified argument to the method. You need to reference the parent ValueObject to fully qualify something as far as I can tell, so you'd have to pass along the parent as well. I think that it sounds nice for the TypeSystem to fully qualify the class name but I don't think that it's necessarily better than what exists now. You'll still have recursion walking the parent ValueObjects, you've just split it across two different methods in different classes. Maybe I'm overthinking this or missing something though?

Just for the type I am saying. Get the fully qualified name of the type. That should be all built into the type in the type system as well right? So no other value would need to be used, just ask the CompilerType for its fully qualified name.

if this wasn't just getting a fully qualified class name then ignore my comments.

labath added inline comments.Jan 14 2020, 12:55 AM
lldb/include/lldb/Symbol/TypeSystem.h
185

I'm confused by the CompilerType argument here. It seems odd as the CompilerType object already contains a type system. Though we seem to have some precedent for this, the majority of TypeSystem functions seems to take an opaque_compiler_type_t (and this seems reasonable to me -- the user would call CompilerType::GetClassName, which would then forward the call to m_type_system->GetClassName(m_opaque_ptr)...).

xiaobai closed this revision.Jan 28 2020, 2:12 PM

Made obsolete by D73517