From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============4002491702588079495==" MIME-Version: 1.0 From: Josh Stone To: elfutils-devel@lists.fedorahosted.org Subject: Re: [PATCH 2/3] Simplify and inline get_uleb128 and get_sleb128 Date: Wed, 23 Apr 2014 12:01:42 -0700 Message-ID: <53580E16.7040001@redhat.com> In-Reply-To: 53580BA7.8050303@redhat.com --===============4002491702588079495== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On 04/23/2014 11:51 AM, Richard Henderson wrote: > On 04/23/2014 11:32 AM, Petr Machata wrote: >> Richard Henderson writes: >> >>> On 04/23/2014 03:17 AM, Petr Machata wrote: >>>> Wouldn't something like this get us off the hook as well? >>>> >>>> - (var) |=3D (typeof (var)) __s.i << ((nth) * 7); \ >>>> + (var) |=3D (typeof (var)) \ >>>> + (((uint64_t) (typeof (var)) __s.i) << ((nth) * 7)); \ >>>> >>>> We are really only using the bitfield trick to avoid having to >>>> sign-extend by hand, but we can shift unsigned without losing anything. >>> >>> It gets us off the hook, but might introduce a 64-bit shift where >>> only a 32-bit shift was required. >> >> Good point, but get_sleb128_step is only used from __libdw_get_sleb128, >> where the type is int64_t. This macro is not safe for outside use >> anyway, as it uses its parameters more than once. >> >> Hmm, should we maybe #undef it after __libdw_get_sleb128, so that it's >> clear that it's for local use only? > = > In that case, why play around with the typeof at all? Just merge the mac= ro > into its only user. At which point using uint64_t makes total sense in c= ontext. > = > Similarly with get_uleb128_step, I assume, at least wrt to the typeof. The way these are macro'd is partly a legacy of earlier code. It does help for the uleb case since unrolling the first step proved to benefit optimization. Keeping sleb this way is helpful to maintain their similarity as much as possible. Sure, the typeof's are probably unnecessary anymore, but this is splitting hairs IMO. It doesn't help anything to drop this, and we could decide to provide narrower _libdw_get... in the future for certain callers. --===============4002491702588079495==--