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 6227 - Disallow comparison of different enums #6444

Closed
wants to merge 2 commits into from
Closed

Fix issue 6227 - Disallow comparison of different enums #6444

wants to merge 2 commits into from

Conversation

LemonBoy
Copy link
Contributor

This is only for named enums, the anonymous ones are currently typed as their own base type and we can't do much about those at least with the way the anonymous enums are handled by the frontend.

I agree it is possible to fix enum.

So let's do it, one step a time!
There are no regressions in phobos or druntime which is a green flag, let's see what you (and the auto tester) has to say about this change.

@dlang-bot
Copy link
Contributor

Fix Bugzilla Description
6227 Comparison of different enums

@WalterWaldron
Copy link
Contributor

Disallow -> Deprecate

@MartinNowak
Copy link
Member

Also a changelog notice that this got deprecated would be nice.
https://github.com/dlang/dmd/tree/master/changelog

@@ -8,7 +8,7 @@ r ARG1 ARG2
echo RESULT=
p args
---
GDB_MATCH: RESULT=.*ARG1.*ARG2
Copy link
Member

Choose a reason for hiding this comment

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

???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. Since the array contains the full path as zero-eth element gdb decides to split the content over many lines, making the regex fail. I've never bothered to submit a fixed test and I guess this is the right moment to do so.

@MartinNowak
Copy link
Member

Seems like it triggered on assignment as well.
Also we should address this not only for equal comparisons, but for any comparisons? What about other operations?

@LemonBoy
Copy link
Contributor Author

Not quite, what you see in diag14950 is this EqualExp being analysed during the calculation of the values for the subsequent enum members and can be easily solved by gagging the errors during the evaluation of the + 1 expression and by making sure the memtype is an integral (or floating) one so that we can show a meaningful error.

Wrt extending this rule to the other binary operations I think that's the next logical step and can be easily added at a later time to this PR, at least once/if the team and community approves this idea.

@LemonBoy
Copy link
Contributor Author

As an added bonus this change allows us to easily fix b16083: in the example provided both the AliasSeq are hashed to the same value since dtemplate.d expressionHash hashes the value itself (the "foo" string) and doesn't take into account the type.

@ordahan
Copy link

ordahan commented May 15, 2017

Hey guys, whats going on with this PR?

@WalterBright
Copy link
Member

One issue with this is if one wants to compare such issues, a cast becomes necessary. Casts are dangerous creatures, in that if the underlying types shift, all kinds of hidden problems can emerge (like unintended truncation).

@andralex
Copy link
Member

We plan to get this fixed. @LemonBoy are you around? Rebase please?

@@ -15682,6 +15682,12 @@ extern (C++) final class EqualExp : BinExp
if (e1.op == TOKtype || e2.op == TOKtype)
return incompatibleTypes();

auto t1 = e1.type;
Copy link
Member

Choose a reason for hiding this comment

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

t1 and t2
are either already used or getting used futher down.
I recommend scopeing { them in a bock }

@andralex
Copy link
Member

Since @LemonBoy seems to not be around, @UplinkCoder could you please take this over? Please let us know. Thanks!

@UplinkCoder
Copy link
Member

@andralex @LemonBoy is around, I just saw him on irc a few days ago
I can fork this pr and fix it; but I'd rather wait a day or two.

@UplinkCoder
Copy link
Member

UplinkCoder commented May 22, 2017

@andralex done in #6821
@LemonBoy told me it's okay to take this over.

@andralex
Copy link
Member

@UplinkCoder thx! @LemonBoy you're always welcome around!

@andralex andralex closed this May 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants