This is an archive of the discontinued LLVM Phabricator instance.

[LLVM-C] [bindings/go] Add C and Golang bindings for COMDAT
ClosedPublic

Authored by ben-clayton on Mar 5 2018, 4:13 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ben-clayton created this revision.Mar 5 2018, 4:13 AM
chandlerc added a subscriber: chandlerc.

Adding Peter who is probably in a good position to review here.

rnk added a comment.Mar 12 2018, 12:04 PM

One issue, looks good otherwise. I don't know Go but it seems straightforward.

include/llvm-c/Comdat.h
24 ↗(On Diff #136969)

These should probably have explicit enumerator values so that they do not change over time.

ben-clayton added inline comments.Mar 13 2018, 2:29 AM
include/llvm-c/Comdat.h
24 ↗(On Diff #136969)

Happy to change, but I'd like to understand the reasoning for keeping the enum values immutable. In Comdat.cpp we map these values to the C++ equivalents, so from at least the LLVM perspective there's no problem with the values changing over time. Are we expecting other languages to hardcode the constants instead of using this C enum?

rnk accepted this revision.Mar 13 2018, 1:38 PM
rnk added inline comments.
include/llvm-c/Comdat.h
24 ↗(On Diff #136969)

I guess we could leave it alone. llvm-c/Core.h uses both numbered and unnumbered enums. It looks like when enumerators are removed, in practice people do transition the enum to something explicitly numbered. I guess the lesson is that maybe we can trust people to be careful when editing these enums, in which case, unnumbered is fine.

This revision is now accepted and ready to land.Mar 13 2018, 1:38 PM

Ack.
Thank you for your review!

I do not have privileges to commit the patch myself.
Would someone be so kind?

This revision was automatically updated to reflect the committed changes.