Page MenuHomePhabricator

[lldb][NFC] Rename ClangASTContext to TypeSystemClang
ClosedPublic

Authored by teemperor on Jan 14 2020, 12:46 AM.

Details

Summary

This commit renames ClangASTContext to TypeSystemClang to better reflect what this class is actually supposed to do
(implement the TypeSystem interface for Clang). It also gets rid of the very confusing situation that we have both a
clang::ASTContext and a ClangASTContext in clang (which sometimes causes Clang people to think I'm fiddling
with Clang's ASTContext when I'm actually just doing LLDB work).

I also have plans to potentially have multiple clang::ASTContext instances associated with one ClangASTContext so
the ASTContext naming will then become even more confusing to people.

Diff Detail

Event Timeline

teemperor created this revision.Jan 14 2020, 12:46 AM

We are currently after a fresh rebranch downstream so now is a good time to rename this without causing merge-conflict hell later on. It would be useful if could get consensus if and to what we should rename this class soon-ish so that I can make this move as painless as possible.

Obviously the contents of this patch are just dummy contents until we agree on a final name. The ClangTypeSystem just seems obvious (and it has the same amount of characters as ClangASTContext so it won't cause any formatting changes when I rename it).

TypeSystemClang would probably be more consistent with how other lldb classes are named (SymbolFileDWARF, ValueObjectChild, etc.), but I don't have a big problem with ClangTypeSystem either...

Both TypeSystemClang and ClangTypeSystem works for me.

teemperor updated this revision to Diff 237953.Jan 14 2020, 5:29 AM
  • Rebased to get rid of shady StringRef -> C-String conversion.

Huh, seems I uploaded to the wrong review

teemperor updated this revision to Diff 237959.Jan 14 2020, 6:14 AM
teemperor retitled this revision from [lldb][NFC] Rename ClangASTContext to ClangTypeSystem to [lldb][NFC] Rename ClangASTContext to TypeSystemClang.
  • Revert to original dummy patch.

I like this idea quite a bit, but have no preference for ClangTypeSystem or TypeSystemClang. +1 from me.

This is very good, go for it. Should we do the same for Swift? cc: @aprantl
For the future, please CC: me directly on these kind of changes if you want my review, as I might miss them otherwise.

I agree it should be TypeSystemClang, since that's how we name all the plugins. Other that that this seems like a great change. We should do the same for swift to keep things consistent, though that's not relevant to this patch.

This is very good, go for it. Should we do the same for Swift? cc: @aprantl

Yeah, otherwise this is going to be confusing.

We might also want to move these into lldb/source/Plugins/TypeSystem as well to complete this refactor?

We might also want to move these into lldb/source/Plugins/TypeSystem as well to complete this refactor?

You could move it now, but ValueObject still depends on it (and you would be introducing more plugin dependencies that shouldn't exist). I have a patch here (D69820) removing it that still needs some more work but after that we should be able to move it without introducing any more dependencies.

We might also want to move these into lldb/source/Plugins/TypeSystem as well to complete this refactor?

I fully agree but I prefer if we could do this as it's own change. This patch is already huge once I renamed all occurrences of ClangASTContext in the source.

teemperor updated this revision to Diff 238191.Jan 15 2020, 1:23 AM
teemperor edited the summary of this revision. (Show Details)
  • Replaced dummy contents with actual patch

Are there any objections against this? Otherwise I'll land this tomorrow.

labath accepted this revision.Jan 16 2020, 6:49 AM

It looks like everyone is on board with this...

This revision is now accepted and ready to land.Jan 16 2020, 6:49 AM
clayborg accepted this revision.Jan 16 2020, 11:50 AM

Looks good. Making TypeSystem's into real plugins in the future would be great too.

xiaobai accepted this revision.Jan 16 2020, 12:50 PM
This revision was automatically updated to reflect the committed changes.