Skip to content

Conversation

hpohl
Copy link
Contributor

@hpohl hpohl commented Aug 8, 2013

@ghost
Copy link

ghost commented Oct 1, 2013

There is an edge case, the following currently works but will fail with your pull:

enum E : int;
static assert(is(E e == enum));

@ghost
Copy link

ghost commented Oct 1, 2013

Using the following works for me:

tded = ((TypeEnum *)targ)->toBasetype();

With these test-cases:

enum E : int;
static assert(is(E e == enum));

enum F;
static assert(is(F e == enum));

Edit: Actually they both pass now, but you may be right that the second case should actually be disallowed.

@ghost
Copy link

ghost commented Oct 1, 2013

@9rnsr: What do you think, should both cases be allowed? enum F's basetype will be int in the second static assert.

@9rnsr
Copy link
Contributor

9rnsr commented Oct 1, 2013

In the enum F;, the base type is unknown. Therefore, I think the first case should be allowed, but the second case should not.

@hpohl
Copy link
Contributor Author

hpohl commented Oct 2, 2013

Updated.

@@ -33,6 +33,7 @@
defaultval = NULL;
sinit = NULL;
isdeprecated = false;
hasexplicittype = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding hasexplicittype looks weird to me.

I had thought that, if !members && !memtype, the taking the base type would cause "forward reference" or "unknown size" error.
However by looking enum.c now, I noticed that EnumDeclaration::getMemtype returns Type::tint32 for the base type of enum E;.

I think that the current EnumDeclaration::getMemtype behavior would be a bug. Could you consider fixing the function rather than adding hasexplicittype field?

@9rnsr
Copy link
Contributor

9rnsr commented Oct 11, 2013

Sorry for late reply.

While testing your PR in my local, I noticed that it would be better not to cause "forward reference" error in TypeEnum::toBasetype() if the enum type is opaque. So it would become:

 Type *TypeEnum::toBasetype()
 {
+    if (!sym->members && !sym->memtype)
+        return this;
     return sym->getMemtype(Loc())->toBasetype();
 }

@9rnsr
Copy link
Contributor

9rnsr commented Oct 13, 2013

Please fix file name: test/fail_compilation/10770.d -> test/fail_compilation/ice10770.d

@9rnsr
Copy link
Contributor

9rnsr commented Oct 13, 2013

@hpohl , Could you please add some comment when you update/push commits? If not, I cannot notice your change (Github doesn't send any notification emails to the review participants in such cases).

@9rnsr
Copy link
Contributor

9rnsr commented Nov 16, 2013

Do you have any plan to fix issue 10770? If not, I'd open a new PR for the ICE bug.

@9rnsr
Copy link
Contributor

9rnsr commented Nov 17, 2013

Posted #2795

@9rnsr
Copy link
Contributor

9rnsr commented Nov 18, 2013

#2795 was merged, so I close this.

@9rnsr 9rnsr closed this Nov 18, 2013
@hpohl
Copy link
Contributor Author

hpohl commented Nov 18, 2013

Sorry, will look into my other PRs next weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants