Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Issue 21932 ImportC: enum ENUM conflicts with enum ENUM #12787

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

WalterBright
Copy link
Member

Fixing the bug meant revamping how enums were done:

  1. Treat them analogously to structs
  2. Fix errors with redeclaring a union tag as an enum tag
  3. Instead of manifest constant declarations, use alias declarations
  4. Give errors for incomplete enum declarations
  5. Fix failure to distinguish union tags from struct tags
  6. Uses D's semantic analyzer for enums, which gives the D side access to things like the .max property

Todo: enum base types are still int. Need to figure out just what gcc's wrongly documented rule is for determining the base type (it doesn't match gcc's behavior). The C11 spec is self-contradictory on this issue.

@WalterBright WalterBright added the ImportC Pertaining to ImportC support label Jun 30, 2021
@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 30, 2021

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
21932 critical importC: enum 'ENUM' conflicts with enum 'ENUM'

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12787"

@WalterBright
Copy link
Member Author

This is not a critical bug. Lowered to normal.

@thewilsonator
Copy link
Contributor

Please fix the typo in the title and commit message.

@WalterBright WalterBright changed the title fix Issue 21932 ImportC: enum ENUM conflicst with enum ENUM fix Issue 21932 ImportC: enum ENUM conflicts with enum ENUM Jun 30, 2021
@WalterBright WalterBright force-pushed the fix21932 branch 3 times, most recently from 942f402 to 794ccbb Compare June 30, 2021 09:53
Comment on lines +2098 to +2100
else
{
}
Copy link
Member

@ibuclaw ibuclaw Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything going to end up here? Otherwise just remove it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left that there just to point out that I had not forgotten about that case.

@ibuclaw
Copy link
Member

ibuclaw commented Jun 30, 2021

Todo: enum base types are still int. Need to figure out just what gcc's wrongly documented rule is for determining the base type (it doesn't match gcc's behavior). The C11 spec is self-contradictory on this issue.

Just use the same logic as how what type an integer literal should be. Though I'm curious how it deals with both a signed and unsigned value as members. With 32-bit in, I assume it would go up to 64-bit long, but from there, would it pick 128-bit cent?

Edit: Seems to be warn and overflow

warning: integer constant is so large that it is unsigned
     |   slong = -9223372036854775808L,
     |            ^~~~~~~~~~~~~~~~~~~~
warning: enumeration values exceed range of largest integer
     | };
     | ^
{
  TestEnum a = -2147483648;
  TestEnum b = 4294967295;
  TestEnum c = -9223372036854775808;
  TestEnum d = -1(OVF);

  return 8;
}

@ibuclaw
Copy link
Member

ibuclaw commented Jun 30, 2021

Just use the same logic as how what type an integer literal should be.

Saying that, we do have IntRange which should allow us to do something like...

const enumrange = IntRange(SignExtendedNumber(ed.minval.getInteger()),
                           SignExtendedNumber(ed.maxval.getInteger()));
if (IntRange.fromType(Type.tuns32).contains(enumrange))
    type = Type.tuns32;
else if (IntRange.fromType(Type.tint32).contains(enumrange))
    type = Type.tint32;
else if (IntRange.fromType(Type.tuns64).contains(enumrange))
    type = Type.tuns64;
else
    type = Type.tint64;

@WalterBright
Copy link
Member Author

I thought I'd use the "common type" logic one gets with A + B + C + D to determine the final type of the entire expression. This would make it consistent and unsurprising.

@WalterBright
Copy link
Member Author

But that's a problem for a separate PR.

@ibuclaw
Copy link
Member

ibuclaw commented Jul 1, 2021

But that's a problem for a separate PR.

Sure, so long as there's room to add support for ARM short enums afterwards, then it will be fine.

assert(sd2); // only tags allowed in tag symbol table
}
}

if (sd && sd2) // `s` is a tag, `sd2` is the same tag
{
if (log) printf(" tag is already defined\n");

if (sd.kind() != sd2.kind()) // being enum/struct/union must match
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, hidden string comparison.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly, as D merges string duplicates.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if they appear in the same compilation unit.

@dlang-bot dlang-bot merged commit fa874a8 into dlang:master Jul 1, 2021
@WalterBright WalterBright deleted the fix21932 branch July 1, 2021 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Bug Fix ImportC Pertaining to ImportC support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants