Page MenuHomePhabricator

[lldb/Utility] Simplify and generalize Scalar class
ClosedPublic

Authored by labath on Aug 12 2020, 7:19 AM.

Details

Summary

The class contains an enum listing all host integer types as well as
some non-host types. This setup is a remnant of a time when this class
was actually implemented in terms of host integer types. Now that we are
using llvm::APInt, they are mostly useless and mean that each function
needs to enumerate all of these cases even though it treats most of them
identically.

I only leave e_sint and e_uint to denote the integer signedness, but I
want to remove that in a follow-up as well.

Removing these cases simplifies most of these functions, with the only
exception being PromoteToMaxType, which can no longer rely on a simple
enum comparison to determine what needs to be promoted.

This also makes the class ready to work with arbitrary integer sizes, so
it does not need to be modified when someone needs to add a larger
integer size.

Event Timeline

labath created this revision.Aug 12 2020, 7:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2020, 7:19 AM
labath requested review of this revision.Aug 12 2020, 7:19 AM
JDevlieghere added inline comments.Aug 12 2020, 8:44 AM
lldb/include/lldb/Utility/Scalar.h
258 ↗(On Diff #285075)

I really hate this signature. How do you feel about having this return a struct with the Type, the temp Scalar and the two references? That doesn't have to be part of this patch though.

labath added inline comments.Aug 13 2020, 6:58 AM
lldb/include/lldb/Utility/Scalar.h
258 ↗(On Diff #285075)

Yeah, that has been bothering me too, but I haven't gotten around to it yet. If I understand what you mean, then the struct solution will not work correctly (without some extra goo) because the address of the temp Scalar will change while it is being returned, invalidating the pointers.

How about something like D85906, which just deals away with all the pointer business?

JDevlieghere accepted this revision.Aug 13 2020, 8:47 AM

I guess you'll have to rebase this, but LGTM

lldb/include/lldb/Utility/Scalar.h
258 ↗(On Diff #285075)

I think with guaranteed copy elision in C++14 it should've been fine, but I agree that it's a bit fragile. I like D85906!

This revision is now accepted and ready to land.Aug 13 2020, 8:47 AM
This revision was landed with ongoing or failed builds.Aug 17 2020, 2:15 AM
This revision was automatically updated to reflect the committed changes.