This is an archive of the discontinued LLVM Phabricator instance.

[ClangExpressionParser] Add ClangDeclVendor
ClosedPublic

Authored by xiaobai on Aug 19 2019, 4:09 PM.

Details

Summary

This introduces a layer between DeclVendor and the currently implemented
DeclVendors (ClangModulesDeclVendor and AppleObjCDeclVendor). This
allows the removal of DeclVendor::GetImporterSource which is extremely
clang-specific, freeing up the interface to be more general.

A good follow up to this would be to remove the remaining instances of
clang in DeclVendor, either by moving things to ClangDeclVendor or by
using wrappers (e.g. CompilerDecl instead of clang::NamedDecl).

Event Timeline

xiaobai created this revision.Aug 19 2019, 4:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2019, 4:09 PM
JDevlieghere added inline comments.Aug 19 2019, 4:30 PM
lldb/include/lldb/Symbol/DeclVendor.h
27

What's eLastClangDeclVendor and where is it used?

lldb/source/Plugins/ExpressionParser/Clang/ClangDeclVendor.h
33

I think a switch would be more readable?

xiaobai marked an inline comment as done.Aug 19 2019, 4:36 PM
xiaobai added inline comments.
lldb/include/lldb/Symbol/DeclVendor.h
27

Following the advice here: https://llvm.org/docs/HowToSetUpLLVMStyleRTTI.html

I added a Last one to indicate that any new ClangDeclVendor subclasses should go before this one. It also makes the classof pattern of checking bounds easier. (kind >= eFoo && kind < eLastFoo).

JDevlieghere added inline comments.Aug 19 2019, 5:27 PM
lldb/include/lldb/Symbol/DeclVendor.h
27

Fair enough, I totally forgot about that pattern.

teemperor accepted this revision.Aug 20 2019, 12:24 AM
teemperor added a subscriber: teemperor.

LGTM

This revision is now accepted and ready to land.Aug 20 2019, 12:24 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2019, 11:46 AM
shafik added a subscriber: shafik.Sep 26 2019, 9:22 AM
shafik added inline comments.
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
110 ↗(On Diff #216201)

This part of the change was recently reverted, see: http://llvm.org/viewvc/llvm-project?view=revision&revision=372974

I am wondering if the change right above for runtime_decl_vendor is also problematic as well. Can you explain the rationale?

CC @teemperor

xiaobai marked an inline comment as done.Sep 26 2019, 11:01 AM
xiaobai added inline comments.
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp
110 ↗(On Diff #216201)

This is actually a bug. I intended to cast it to a ClangDeclVendor. The instance above for runtime_decl_vendor shouldn't be problematic. The rationale behind this change was to remove the clang-specific code in the DeclVendor class by introducing a ClangDeclVendor class to sit in between DeclVendor and its clang-specific subclasses (ClangModulesDeclVendor and AppleObjCDeclVendor). That partial revert is probably fine honestly since it's using ClangModulesDeclVendor directly.