[Lldb-commits] [PATCH] D64844: [Target] Remove Target::GetScratchClangASTContext

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64844/new/

https://reviews.llvm.org/D64844