Page MenuHomePhabricator

Add support for owning module information to TypeSystemClang.

Authored by aprantl on Mar 4 2020, 10:25 AM.



This was split out from

Diff Detail

Event Timeline

Thanks for splitting this up. This seems fine to me. I'll leave the approval to the type system folks.

I don't know how much we care about the size of the Type class. If we did, there are ways this could be optimized. But I guess the fact that we stored m_is_complete_objc_class as a lone bool flag (and so changing that to a uint32_t doesn't cost anything) means that we don't really care that much about it.


bad formatting


here too

I'm not a big fan of storing data in the ClangExternalASTSourceCallbacks. The main reason is that this external source only exists when the ASTContext in the TypeSystemClang is created by the TypeSystemClang. When the TypeSystem adopts an ASTContext there can be any (or none) ExternalASTSource so GetOrCreateClangModule would crash/assert. ClangExternalASTSourceCallbacks always knows its respective TypeSystemClang, so storing the data in TypeSystemClang and then getting it from there when getSourceDescriptor/getModule is called will always work (and saves you from RTTI-casting/checking the ExternalASTSource).


All the m_* variables that correspond to a part of ASTContext are only initialized when the TypeSystemClang has created its own ASTContext, so this code would crash if that's not the case. Calling getASTContext().getX() is the way that always works.


Could we change this (and all similar parameters) from an unsigned to at one of those:

  • A typedef such as typedef unsigned ModuleID. It seems to be optional, so llvm::Optional<ModuleID> would be even better so I could pass llvm::None instead of 0.
  • (preferred) A new ModuleID type that doesn't implicitly construct from unsigned. Many of these functions already take some kind of integer and I would prefer if we don't add more.

That would also make the GetOrCreateClangModule self-documenting.


Could this be declared next to GetOrCreateClangModule? This is currently declared in the middle of the CompilerDeclContext backend (which no longer makes so much sense with the new version of the patch).


The correct way to get the SourceManager and LangOpts is to do getASTContext().getSourceManager(). m_language_options_up, m_source_manager_upetc. are only set if the TypeSystemClang created its own ASTContext but are null if the TypeSystemClang adopted an ASTContext (e.g., in an expression).

This can also return a reference instead of a pointer. I removed the option to have an empty TypeSystemClang.

shafik added inline comments.Mar 10 2020, 4:46 PM

Why not use uint32_t like we did above? If we are going to assume 32 bits we should just use the fixed width type.

shafik added inline comments.Mar 10 2020, 10:21 PM

We are passing around 0 everywhere and it is not obvious what it means at all.

Can we name this somehow or make it a type?

aprantl updated this revision to Diff 250938.Mar 17 2020, 4:09 PM
aprantl marked 5 inline comments as done.

Addressed review feedback and made the whole thing more type-safe.

aprantl updated this revision to Diff 250941.Mar 17 2020, 4:22 PM
labath added inline comments.Mar 18 2020, 6:10 AM
146 ↗(On Diff #250941)

I am kinda lost at this point, so I'm not sure which patch introduces this, but... this name does not seem very ideal, as it sounds like it is related to lldb_private::Module, while it does not really have that much in common with it.

And then there's ClangModulesDeclVendor::ModuleID -- I have no idea what's the relationship of this to that..

aprantl abandoned this revision.Mar 18 2020, 8:56 AM
aprantl marked an inline comment as done.
aprantl added inline comments.

This is just an intermediate step and gets replaced in

146 ↗(On Diff #250941)

I agree. I'm going to clean this up some more and I will also merge this review into because it doesn't actually make reviewing easier to see these API changes without their use.


That is consistent with the type Clang uses for module IDs.

I'm merging this back into for easier reviewing.

And then there's ClangModulesDeclVendor::ModuleID -- I have no idea what's the relationship of this to that..

ClangModulesDeclVendor::ModuleID is a typedef to clang::Module * so it is a bit of a misnomer, since clang calls the unsigned module numbers module ids.