Page MenuHomePhabricator

[lldb] Replace host-typed Scalar accessors with [SZ]ExtOrTruncInt

Authored by labath on Jun 30 2020, 4:24 AM.



The Scalar class gives off the impression that it follows the C type
conversion rules, but this is not true for its host-typed accessor
methods. For example, Scalar((int)-1).ULongLong() returns 0xffffffff,
but (unsigned long long)(int)-1 is 0xffffffffffffffff. I.e., Scalar's
ULongLong always zero-extends, but C semantics require sign-extension.

This patch renames [SU]LongLong to [SZ]ExtOrTruncInt to better reflect
the conversion being done (name chosen after the underlying APInt
operation). Accessors for smaller types are removed completely as the
are not necessary -- normal C conversions from (u)int64_t will handle
that for us in most cases.

Diff Detail

Event Timeline

labath created this revision.Jun 30 2020, 4:24 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: emaste. · View Herald Transcript
mib added a comment.EditedJun 30 2020, 8:36 AM

Looks good to me apart from one small detail.

I'll leave it open for others to comment on this, since there are lots of changes.


Any reason why this is not inlined in the definition ?

labath added a comment.Jul 3 2020, 6:30 AM

Not relevant now that D82772 is in. It still may make sense to revisit this idea later.

jankratochvil added inline comments.

IMO this patch does only the renaming, it should not do unrelated code cleanups.
For example by s/ZExtOrTruncInt/ULongLong/ s/SExtOrTruncInt/SLongLong/ + clang-format one should get zero-sized patch to verify there are no unintended changes.

jankratochvil requested changes to this revision.Oct 29 2020, 2:47 AM

This patch no longer applies, it needs a rebase.

This revision now requires changes to proceed.Oct 29 2020, 2:47 AM
labath abandoned this revision.Oct 29 2020, 5:09 AM

Just abandoning. I believe I've removed the most egregious parts of the Scalar class now. There's still room for improvement, but that requires a fresh start anyway...