This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Reduce some dispatch boilerplates. NFC.
ClosedPublic

Authored by timshen on Jan 25 2017, 5:03 PM.

Details

Summary

This is an attempt to reduce the verbose manual dispatching code in APFloat. This doesn't handle multiple dispatch on single discriminator (e.g. APFloat::add(const APFloat&)), nor handles multiple dispatch on multiple discriminators (e.g. APFloat::convert()).

Diff Detail

Repository
rL LLVM

Event Timeline

timshen created this revision.Jan 25 2017, 5:03 PM
jlebar accepted this revision.Jan 26 2017, 10:27 AM

Other than the naming nit, this seems like an improvement to me.

llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

What does "single" refer to here?

APFLOAT_SEMANTIC_DISPATCH? I don't really like that either.

Maybe someone else has a better suggestion.

This revision is now accepted and ready to land.Jan 26 2017, 10:27 AM
echristo added inline comments.Jan 26 2017, 10:29 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

APFLOAT_FORMAT_DISPATCH ?

jlebar added inline comments.Jan 26 2017, 10:31 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

I like that one.

timshen added inline comments.Jan 26 2017, 10:34 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

For single dispatch, see https://en.m.wikipedia.org/wiki/Dynamic_dispatch#Single_and_multiple_dispatch .

Here the macro expands to code that dispatches on "*this" APFloat object. This isn't the case for add or convert.

echristo added inline comments.Jan 26 2017, 10:37 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

Most people will read the macro as a "what are we doing" rather than "how are we doing it" I think which is why I suggested format.

timshen added inline comments.Jan 26 2017, 10:39 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

I actually don't know what "format" means here. How about APFLOAT_DISPATCH? :)

echristo added inline comments.Jan 26 2017, 10:41 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

You could also use layout dispatch. The idea is the "format" of the float. I just don't like layout, but that's also fine if you'd prefer it.

timshen added inline comments.Jan 26 2017, 10:46 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

I see. Some how "format" reminded me of format string... oh well.

Then the consistent term to use here is "semantics". How about APFLOAT_DISPATCH_ON_SEMANTICS?

jlebar added inline comments.Jan 26 2017, 11:05 AM
llvm/include/llvm/ADT/APFloat.h
24 ↗(On Diff #85843)

I'm also fine with that.

timshen updated this revision to Diff 85962.Jan 26 2017, 1:43 PM

Change macro name to APFLOAT_DISPATCH_ON_SEMANTICS.

This revision was automatically updated to reflect the committed changes.