Skip to content

Conversation

kuilpd
Copy link
Collaborator

@kuilpd kuilpd commented Apr 23, 2025

Move type checks for C-style, static, reinterpret casts. Remove dynamic cast checks, since it's not implemented anyway.

@kuilpd kuilpd requested a review from cmtice April 23, 2025 17:18
@kuilpd
Copy link
Collaborator Author

kuilpd commented Apr 23, 2025

@cmtice
I have to say, this whole thing is a mess. Compared to unary/binary operators, I couldn't really unify the type checking code with the evaluating code, so it doesn't look good at all. Plus, I had to leave a small type check in the parser to determine whether the node is an rvalue or not, since this can't really be changed later during evaluation.
I suspect that when we get to these nodes in the upstream, the suggestion will be to throw most of the checks away and just call an appropriate rhs->CastTo...(type);, and implement other necessary code in ValueObject.cpp, so I don't think there's need to optimize the code here now.

@cmtice
Copy link
Collaborator

cmtice commented May 7, 2025

The ValueObject class already contains many member functions for doing type casting -- we should probably be using them as much as possible (and letting them do most of the error checking. In ValueObject.h, I see:

lldb::ValueObjectSP Cast(const CompilerType &compiler_type);
virtual lldb::ValueObjectSP DoCast(const CompilerType &compiler_type);
virtual lldb::ValueObjectSP CastPointerType(const char *name, CompilerType &ast_type)
virtual lldb::ValueObjectSP CastPointerType(const char *name, lldb::TypeSP &type_sp);
llvm::Expected<lldb::ValueObjectSP> CastDerivedToBaseType(CompilerType type, const llvm::ArrayRef<uint32_t> &base_type_indices);
llvm::Expected<lldb::ValueObjectSP> CastBaseToDerivedType(CompilerType type, uint64_t offset);
lldb::ValueObjectSP CastToBasicType(CompilerType type);
lldb::ValueObjectSP CastToEnumType(CompilerType type);

I suspect doing this would simplify the code a lot.

@cmtice
Copy link
Collaborator

cmtice commented May 7, 2025

Oh my bad! It looks like you did you a lot of those after all. But I'm not quite clear if you are doing extra, unnecessary error checking? Maybe you can rely on those functions for more of the correctness error checking?

Also, I think we should eliminate all of the C++-specific types of casts, I.e. Don't do or allow CxxStaticCast, CxxReinterpretCast or CxxDynamicCast. Only do CStyleCast. I think that should also simplify things quite a bit.

@kuilpd
Copy link
Collaborator Author

kuilpd commented May 7, 2025

Oh my bad! It looks like you did you a lot of those after all. But I'm not quite clear if you are doing extra, unnecessary error checking? Maybe you can rely on those functions for more of the correctness error checking?

Also, I think we should eliminate all of the C++-specific types of casts, I.e. Don't do or allow CxxStaticCast, CxxReinterpretCast or CxxDynamicCast. Only do CStyleCast. I think that should also simplify things quite a bit.

Honestly, I just copied most of the code that was in the Parser and adjusted it to work with interfaces available in Eval, didn't look further into optimizing type checking. With the direction our upstream pull requests are going, it seems like there will be barely any code left similar to our downstream by the time we get to casts, so we can do optimization when we get there. For now I just wanted the type checking contained in the parser regardless of its efficiency.
I'd say let the C++ casts also be for now, so that the downstream has every capability we implemented at least as a reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants