This is an archive of the discontinued LLVM Phabricator instance.

[Target] Remove Process::GetObjCLanguageRuntime
ClosedPublic

Authored by xiaobai on Jun 8 2019, 4:02 PM.

Details

Summary

In an effort to make Process more language agnostic, I removed
GetCPPLanguageRuntime from Process. I'm following up now with an equivalent
change for ObjC.

Event Timeline

xiaobai created this revision.Jun 8 2019, 4:02 PM
xiaobai updated this revision to Diff 203714.Jun 8 2019, 4:11 PM

Small cleanups

compnerd added inline comments.Jun 9 2019, 12:47 PM
include/lldb/Target/ObjCLanguageRuntime.h
202

I think it would be nice to just call this Get (and we could have equivalents in the other languages). It makes the uses less verbose and repetitive.

source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
515–517

Why not create a variable?

if (const auto *runtime = ObjCLanguageRuntime::Get(*process))

labath added inline comments.Jun 10 2019, 12:13 AM
include/lldb/Target/ObjCLanguageRuntime.h
202

I like that idea.

xiaobai marked an inline comment as done.Jun 10 2019, 11:28 AM
xiaobai added inline comments.
include/lldb/Target/ObjCLanguageRuntime.h
202

I also like this idea. Would you mind if I did that in a follow up?

xiaobai updated this revision to Diff 203872.Jun 10 2019, 12:16 PM

Simplify a change

labath added inline comments.Jun 10 2019, 12:22 PM
include/lldb/Target/ObjCLanguageRuntime.h
202

You're introducing the function in this patch. What's the point in renaming it immediately after that?

I can see how renaming GetCPPLanguageRuntime might be a good thing to do in a separate patch, but I don't see a reason to do that with this function

xiaobai marked an inline comment as done.Jun 10 2019, 12:26 PM
xiaobai added inline comments.
include/lldb/Target/ObjCLanguageRuntime.h
202

Fair enough. I'll update this patch.

xiaobai updated this revision to Diff 203877.Jun 10 2019, 12:45 PM

ObjCLanguageRuntime::GetObjCLanguageRuntime -> ObjCLanguageRuntime::Get

compnerd accepted this revision.Jun 10 2019, 1:37 PM
This revision is now accepted and ready to land.Jun 10 2019, 1:37 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 10 2019, 1:52 PM