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.
Some minor comments to make this more NFC.
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).
ClangUtil::IsClangType(compiler_type) seems like a more complete check. Then you can also drop the lang_type parameter.
One comment on a part you didn't change, but left a C++ specific track in the generic ValueObject code.
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.
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.
Let me know what everyone thinks of adding a "fully_qualified" argument to the TypeSystem::GetClassName()?
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
bool fully_qualified = true; llvm::Optional<std::string> class_name = type_system->GetClassName(GetCompilerType(), fully_qualified); if (class_name) s.PutCString(class_name.getValue());
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
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.
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)...).