[llvm] r303011 - [StringExtras] Add llvm::to_integer.

[Justin Bogner via llvm-commits]

David Blaikie <dblaikie at gmail.com> writes:
> On Fri, Jun 16, 2017 at 4:05 PM Zachary Turner via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> On Fri, Jun 16, 2017 at 3:41 PM Justin Bogner <mail at justinbogner.com>
>> wrote:
>>
>>> I'm a little late to the discussion, but I pretty strongly agree with
>>> Rui's suggestion that returning an Optional is a better interface here.
>>> I'm not really convinced that the cases where we can actually deduce a
>>> type come up often enough that there's a significant useability
>>> improvement.
>>>
>>
>> I'm still not real crazy about it.  I don't like having to indent to
>> handle the *success* case,
>>
>
> Don't have to, but yeah - does add some extra variables or indirection
> syntax if it's done the other way. Eventually this'll get better with
> coroutines.

To add to this, using the Optional return in the unindented case
basically makes the code look exactly like the argument-return case (but
it's harder to get wrong):

  void return_via_arg(StringRef S) {
      int X;
      if (!to_integer(S, X))
          return;
      // use X
  }
  
  void return_via_optional(StringRef S) {
      auto X = to_int_O(S);
      if (!X)
          return;
      // use *X
  }

>> which you would see a lot of with an Optional<T> return and which goes
>> against our coding standard guidelines of early return.  There's also the
>> issue of size.  For example, an Optional<uint64_t> is 16 bytes, so it has
>> non-zero performance cost
>
> As do out parameters, of course - breaking aliasing assumptions/etc by
> leaking the address of a local out into the rest of the program.

The size difference here should be negligible - the ABI probably returns
the bool in one 64 bit register and the int in another anyway, so we're
using 16 bytes either way.

>> and questionable style benefits.
>
> I think the readability/self-documenting code of having single returns &
> avoiding out parameters where it fits, is pretty high up there.

+1

> That's my take on it, at least.
>
> - Dave