Page MenuHomePhabricator

[Target] Remove Target::GetScratchClangASTContext
Needs RevisionPublic

Authored by xiaobai on Jul 16 2019, 6:33 PM.

Details

Summary

Target doesn't really need to know about ClangASTContext more than any
other TypeSystem. We can create a method ClangASTContext::GetScratch for
anything who needs a ClangASTContext specifically instead of just a
generic TypeSystem.

Event Timeline

xiaobai created this revision.Jul 16 2019, 6:33 PM
xiaobai updated this revision to Diff 210229.Jul 16 2019, 6:35 PM

Remove ClangASTContext header from Target.cpp

This revision is now accepted and ready to land.Jul 16 2019, 11:38 PM
jingham requested changes to this revision.EditedJul 17 2019, 10:23 AM

This seems partly wrong to me. The point is that the Target holds scratch AST contexts for all the languages it supports. They are the central repository for the accumulation of effects of expressions evaluated while that target is alive. For instance, all the user defined types and variables go there. The Target also manages its lifecycle. For instance, in the swift case if there's a module that we can't import, we have to fall back to making an AST Context for each module we can successfully import and dispatching expressions to the appropriate one of those.

So all the Scratch AST Contexts are properly owned by the target.

This move doesn't keep all the clients from knowing they are getting their hands on a ClangASTContext, so it doesn't seem like it achieves much hiding. All it does is conceal the ownership of the scratch AST context for C-family languages.

This revision now requires changes to proceed.Jul 17 2019, 10:23 AM

This seems partly wrong to me. The point is that the Target holds scratch AST contexts for all the languages it supports. They are the central repository for the accumulation of effects of expressions evaluated while that target is alive. For instance, all the user defined types and variables go there. The Target also manages its lifecycle. For instance, in the swift case if there's a module that we can't import, we have to fall back to making an AST Context for each module we can successfully import and dispatching expressions to the appropriate one of those.

So all the Scratch AST Contexts are properly owned by the target.

This move doesn't keep all the clients from knowing they are getting their hands on a ClangASTContext, so it doesn't seem like it achieves much hiding. All it does is conceal the ownership of the scratch AST context for C-family languages.

Right, I'm not trying to remove Target's ownership of any scratch type systems. My goal is to remove Target's knowledge about specific scratch type systems. Target shouldn't have to know about ClangASTContext specifically. It should hand out scratch TypeSystems based on what language you give it which it already does today with GetScratchTypeSystemForLanguage. I'm not trying to hide this information from clients, I'm trying to hide the knowledge from Target.

jingham added a comment.EditedJul 17 2019, 12:11 PM

I think I understand your goal - a worthy one, BTW... But I think this is an unnecessary step along that path.

After all, all the clients of the Target's Scratch TypeSystem for C-family languages should be able to do what they need to do with a TypeSystem, rather than a ClangASTContext. And that's really your goal. It doesn't help much if Target doesn't need to know about ClangASTContext but everyone else (breakpoints, watchpoints, the LanguageRuntimes, etc.) who uses the C-family language Scratch TypeSystem does.

So you don't want to just move Target::GetScratchClangASTContext around, you want to get rid of it. Everybody who was calling Target::GetScratchClangASTContext should call Target::GetScratchTypeSystemForLanguage and that should be sufficient for them.

So a better approach, it seems to me, would be to remove Target::GetScratchClangASTContext altogether, convert all it's callers to Target::GetScratchTypeSystemForLanguage and try changing the type the callers receive from ClangASTContext * to TypeSystem *. Then if any of them ACTUALLY needed to cast this from a TypeSystem to a ClangASTContext, you can change the type back to ClangASTContext, do the cast in situ and add a FIXME: You should be able to do this with a TypeSystem.

That has the - to me - desirable benefit of keeping it clear that these scratch type systems are things handed out by the target, which your change makes less clear. Plus it will quickly show us what is missing from TypeSystem. Or maybe, you will find that for most uses, a TypeSystem WAS good enough, which will be double goodness!

Seems like in most places we're just pulling out basic types or their sizes, which we should certainly be able to do with a TypeSystem.

Yes, I agree that replacing ClangASTContext uses with TypeSystem would be the right thing to do, and it's what I plan on doing next. There are instances where you really do want a ClangASTContext (e.g. in plugins related to clang expression parsing and objc), and so having a convenience function like this means you don't have to cast the result of every call. This is similar to ObjCLanguageRuntime::Get. I don't mind abandoning this patch though.

Luckily, like you pointed out, most uses of ClangASTContext are very generalizable so it shouldn't be too bad to change things over.

The nice thing about the way the ObjCLanguageRuntime::Get was that is was only useable where we decided it was legit to use it, in the actual ObjCLanguageRuntime plugin or its direct users. If you want to keep the convenience cast function in a header in Plugins/ExpressionParser/Clang, that would be fine.