Page MenuHomePhabricator

[Expression] Add PersistentExpressionState::GetCompilerTypeFromPersistentDecl
ClosedPublic

Authored by xiaobai on Jun 2 2019, 4:44 PM.

Details

Summary

PersistentStateExpressions (e.g. ClangPersistentVariables) have the
ability to define types using expressions that persist throughout the
debugging session. GetCompilerTypeFromPersistentDecl is a useful
operation to have if you need to use any of those persistently declared types,
like in CommandObjectMemory.

This decouples clang from CommandObjectMemory and decouples Plugins from
Commands in general.

Diff Detail

Repository
rL LLVM

Event Timeline

xiaobai created this revision.Jun 2 2019, 4:44 PM
compnerd added inline comments.Jun 2 2019, 5:27 PM
source/Commands/CommandObjectMemory.cpp
479 ↗(On Diff #202626)

Why is the parameter clang_ast_type and not based on persistent_vars->getKind()?

source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
58 ↗(On Diff #202626)

NIT: decl would be a nicer name

xiaobai marked an inline comment as done.Jun 2 2019, 5:29 PM
xiaobai added inline comments.
source/Commands/CommandObjectMemory.cpp
479 ↗(On Diff #202626)

clang_ast_type is of type CompilerType. I was going to change the name in a follow up but I can do it in this change too.

labath added a comment.Jun 3 2019, 1:28 AM

The idea seems right, but I'm not too familiar with this part of the code. Someone else ought to look at this too.

include/lldb/Expression/ExpressionVariable.h
235–236 ↗(On Diff #202626)

It would be better if this would return Optional(or Expected)<CompilerType>

xiaobai marked an inline comment as done.Jun 3 2019, 10:04 AM
xiaobai added inline comments.
include/lldb/Expression/ExpressionVariable.h
235–236 ↗(On Diff #202626)

Let me try that out, I'll update this later.

xiaobai updated this revision to Diff 202805.Jun 3 2019, 3:01 PM

Pavel's suggestion
Renamed method from "SetCompilerTypeFromPersistentDecl" to "GetCompilerTypeFromPersistentDecl"

xiaobai retitled this revision from [Expression] Add PersistentExpressionState::SetCompilerTypeFromPersistentDecl to [Expression] Add PersistentExpressionState::GetCompilerTypeFromPersistentDecl.Jun 3 2019, 3:01 PM
xiaobai edited the summary of this revision. (Show Details)
compnerd accepted this revision.Jun 3 2019, 5:56 PM
compnerd added a subscriber: clayborg.

Would be nice to get someone like @clayborg to chime in, but, I think that @labath also seems to think that this is fine.

source/Commands/CommandObjectMemory.cpp
476 ↗(On Diff #202805)

I think that auto would be fine given that the GetPersistentExpressionStateForLanguage spells out the return type.

481 ↗(On Diff #202805)

This seems wrong now. You iterate over all the languages, but always set the clang_ast_type? The type may be a Swift AST type or Clang AST type (or something more exotic like a rust AST type). We should rename that to m_ast_type as per the LLDB style. But, that makes sense to do as a follow up change.

This revision is now accepted and ready to land.Jun 3 2019, 5:56 PM

I have no problem with the change in general. However, you've introduced the possibility of name collision between convenience type definition in various languages. So it would be good to run through the persistent DECL's for ALL the supported languages and report collisions. Probably also need to add a -l flag to pick a language?

I have no problem with the change in general. However, you've introduced the possibility of name collision between convenience type definition in various languages. So it would be good to run through the persistent DECL's for ALL the supported languages and report collisions. Probably also need to add a -l flag to pick a language?

Hmm, yes that's true. Would you like me to implement that in this patch as well or would you be okay with a follow up patch?

Pretty much the only actual effect of this patch (besides its changes to the dependency graph) are introducing the possibility for ambiguity here. It seems more logical to do it all as one patch.

xiaobai updated this revision to Diff 203483.Jun 6 2019, 8:15 PM

Renamed clang_ast_type to compiler_type to be more general.
Allow for disambiguating by language with the flag -x or --language.

JDevlieghere accepted this revision.Jun 11 2019, 2:20 PM
JDevlieghere added inline comments.
source/Plugins/ExpressionParser/Clang/ClangPersistentVariables.cpp
66 ↗(On Diff #203483)

I think there's a matching constructor, which would allow you to get rid of the temporary and do

return CompilerType(ClangASTContext::GetASTContext(&tdecl->getASTContext()),reinterpret_cast<lldb::opaque_compiler_type_t>(const_cast<clang::Type *>(tdecl->getTypeForDecl())));
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2019, 10:44 AM