Apply all the changes from upstream review comments on other DIL PRs.#27
Apply all the changes from upstream review comments on other DIL PRs.#27
Conversation
Major Changes: - Renamed DILASTNode to ASTNode. - Renamed DILInterpreter to Interpreter - Update Parser to have public static 'Parse' function that calls the private parser constructor then calls Run. Returns either an AST tree or an error. - Updated DILEval (and all the Visit funcitons) to return an LLVM::Expected<ValueObjectSP>. - Removed the m_error and m_result member variables from the Interpreter; just directly return the values or the errors. - Updated the calls to the parser & interpreter in DILGetValueForExpressionPath appropriately. - Moved the following class & function declarations & defintions from DILAST to DILEval: struct MemberInfo, GetDynamicOrSyntheticValue, LookupIdentifier, LookupGlobalIdentifier, LookupStaticIdentifier, GetFieldWithNameIndexPath, GetMemberInfo, GetEnumMembers, ResolveTypeByName, DILFindVariable - Changed return type for all ASTNode::Accept() methods to LLVM::Expected<lldb::ValueObjectSP>. - Changed return type for all Visitor::Visit() methods to LLVM::Expected<lldb::ValueObjectSP>. - Removed unused Interpreter constructors. - Update Parser & Interpreter classes to directly use Stack Frame shared ptr for their context scope (passed into the constructors), for their execution context scopes. - Use llvm format providers rather than our own custom string formatters for Token::Kind , Token, TypeDeclaration::TypeSpecifier, and TypeDeclaration::SignSpecifier. - Remove m_dil_token from Parser class; use method 'CurToken()' in its place. - Remove ConsumeToken method (replace it with DILLexer::Advance()). - Make m_error in the Parser class a reference, which gets initialized from the static Parse method (ensuring a very limited lifetime scope for the error). - Modify SetUbStatus to return an error; modify callers to check & handle the error, and to have an LLVM::Expected<...> return type. - Check the error value after calls to ValueObject::Dereference. - Remove SetError from Interpreter; replace with direct setting & returning of errors. Minor Changes: - Simplify the set up in most of the Python tests. - Removed FromContextArg() & GetType() from IdentifierInfo - Remove all special handling for "this" (including kw_this). - Ran clang-format over all the changes => cleaned up the formatting.
|
I think this covers all the changes so far. It's failing 3 of the unittests now -- I'll try to look into that this weekend |
kuilpd
left a comment
kuilpd
left a comment
There was a problem hiding this comment.
The test at DILTests.cpp:1169 can just now expect an address value of this, since it's now treated as a local variable.
| // Last resort, lookup as a register (e.g. `rax` or `rip`). | ||
| if (!value) { |
There was a problem hiding this comment.
TestRegistersNoDollar fails because this was removed during transfer, but I'm guessing this shouldn't work like this anyway, better to remove the test.
There was a problem hiding this comment.
I've Disabled the TestRegistersNoDollar (and re-enabled TestRegisters, which is passing).
| if (m_result->GetCompilerType().IsReferenceType()) | ||
| m_result = m_result->Dereference(error); |
There was a problem hiding this comment.
I fixed up TestMemberOf in #21, this check is needed for the failed tests there.
There was a problem hiding this comment.
I've added that back in. I also found that TestAddressOf was failing because it was expecting to get an rvalue error on '&this', but we no longer treat 'this' as anything special. So I commented out that one line of the test and it's now passing.
So with my update all the unit tests are passing now.
There was a problem hiding this comment.
Let's just remove that test then, otherwise LGTM!
| if (m_result->GetCompilerType().IsReferenceType()) | ||
| m_result = m_result->Dereference(error); |
There was a problem hiding this comment.
Let's just remove that test then, otherwise LGTM!
2 participants
Major Changes:
- Renamed DILASTNode to ASTNode.
- Renamed DILInterpreter to Interpreter
- Update Parser to have public static 'Parse' function that calls the private parser constructor then calls Run.
Returns either an AST tree or an error.
- Updated DILEval (and all the Visit funcitons) to return an
LLVM::Expected.
- Removed the m_error and m_result member variables from the Interpreter; just
directly return the values or the errors.
- Updated the calls to the parser & interpreter in DILGetValueForExpressionPath
appropriately.
- Moved the following class & function declarations & defintions from DILAST
to DILEval: struct MemberInfo, GetDynamicOrSyntheticValue, LookupIdentifier,
LookupGlobalIdentifier, LookupStaticIdentifier, GetFieldWithNameIndexPath,
GetMemberInfo, GetEnumMembers, ResolveTypeByName, DILFindVariable
- Changed return type for all ASTNode::Accept() methods to
LLVM::Expectedlldb::ValueObjectSP.
- Changed return type for all Visitor::Visit() methods to
LLVM::Expectedlldb::ValueObjectSP.
- Removed unused Interpreter constructors.
- Update Parser & Interpreter classes to directly use Stack Frame shared ptr
for their context scope (passed into the constructors), for their
execution context scopes.
- Use llvm format providers rather than our own custom string formatters for
Token::Kind , Token, TypeDeclaration::TypeSpecifier, and
TypeDeclaration::SignSpecifier.
- Remove m_dil_token from Parser class; use method 'CurToken()' in its place.
- Remove ConsumeToken method (replace it with DILLexer::Advance()).
- Make m_error in the Parser class a reference, which gets initialized from the
static Parse method (ensuring a very limited lifetime scope for the error).
- Modify SetUbStatus to return an error; modify callers to check & handle the
error, and to have an LLVM::Expected<...> return type.
- Check the error value after calls to ValueObject::Dereference.
- Remove SetError from Interpreter; replace with direct setting & returning of
errors.