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

Enum values in switch() #41

Closed
fbaro opened this issue Mar 9, 2016 · 18 comments
Closed

Enum values in switch() #41

fbaro opened this issue Mar 9, 2016 · 18 comments
Assignees
Labels
Milestone

Comments

@fbaro
Copy link
Contributor

fbaro commented Mar 9, 2016

When generating a switch on an enum, the case labels should not include the enum class name, but only the enum label.

    public enum E { A, B }
    ...
    switch (e) {
        case A: // Not case E.A
        ...
    }

It's an old issue with the original codemodel.
Unfortunately, I have no idea how to fix this with the generate() method architecture. I solved it on my local codemodel copy with an ugly instanceof in JCase.

@phax
Copy link
Owner

phax commented Mar 11, 2016

Being on vacation. Fixing this afterwards. Should be easy.

phax added a commit that referenced this issue Apr 17, 2016
@phax
Copy link
Owner

phax commented Apr 17, 2016

Please verify if it works for you as well. I remove emitting the type in the JEnumConstant class only.

@phax phax closed this as completed Apr 17, 2016
mcoblenz added a commit to mcoblenz/Obsidian that referenced this issue Apr 4, 2017
@mcoblenz
Copy link

mcoblenz commented Apr 4, 2017

Unfortunately this change breaks code generation in my project because it results in emitting an unscoped enum name in a case where the fully-qualified name is required. The generated code looks like this:

public class Simple4 {
  private State_Simple4 __state;

 private void __conserveFields(State_Simple4 nextState) {
        if (getState() == S1) { // need to be State_Simple4.S1
           …
        }
    }
    public enum State_Simple4 {
        S1,
        S2;
    }
}

@phax phax reopened this Apr 4, 2017
@phax
Copy link
Owner

phax commented Apr 4, 2017

Agreed - had the same issue myself in other scenarios.

@phax phax self-assigned this Apr 4, 2017
@phax phax added the bug label Apr 4, 2017
@phax
Copy link
Owner

phax commented Apr 4, 2017

Do you need it in the 2.8.x series, or can you use Java 8 with upcoming V3?

@mcoblenz
Copy link

mcoblenz commented Apr 4, 2017

V3 is probably fine. For now I'm using a locally-modified V3 and it seems to work okay otherwise. Thanks!

@phax
Copy link
Owner

phax commented Apr 4, 2017

So you basically reverted the changes mentioned in the above referenced commit - right?

@mcoblenz
Copy link

mcoblenz commented Apr 4, 2017

Yes, though I realize that's not the right fix. Presumably we need to check to see what scope the enum was defined in and emit the right thing?

@phax
Copy link
Owner

phax commented Apr 4, 2017

Yep :)

@phax
Copy link
Owner

phax commented May 17, 2017

@mcoblenz So the current solution should suffice both requirements - the difference is done in JCase for enum constants :|

@phax phax added this to the 3.0.0 milestone May 17, 2017
@phax phax closed this as completed May 17, 2017
@mushaorc
Copy link

3.0.0 apparently is back to fully qualified enumeration constants in switch.

phax added a commit that referenced this issue Aug 18, 2017
@phax
Copy link
Owner

phax commented Aug 18, 2017

I added a quick test and it prints:

-----------------------------------issue41.Issue41Test.java-----------------------------------
package issue41;

public class Issue41Test {

    void dummy(Issue41Test.MyEnum enumParam) {
        switch (enumParam) {
            case A:
            {
                break;
            }
            case B:
            {
                break;
            }
            default:
            {
                break;
            }
        }
    }

    public enum MyEnum {
        A,
        B,
        C;
    }
}

==> so to me it looks good...

@mushaorc
Copy link

ok, I probably need to minimize it, but Im switching version 2.8.6 and 3.0.0 for existing project and I have case A: w 2.8.6 and case Enum.A: for 3.0.0

@phax
Copy link
Owner

phax commented Aug 18, 2017

Are you using it like this:

...
  final JEnumConstant ca = jEnumClass.enumConstant ("A");
 ...
  final JSwitch s = method.body ()._switch (p);
...
    s._case (ca).body ()._break ();
...

or do you use the _case with a generic JExpr.ref?

@mushaorc
Copy link

mushaorc commented Aug 18, 2017

JEnumConstant enumEntry(JDefinedClass enumClass, String valueName, int value, String description) {
JEnumConstant enumConstant = enumClass.enumConstant(valueName);
enumConstant.arg(JExpr.lit(value));
return enumConstant;
}

    JSwitch switchVF = valueOfIntMethod.body()._switch(numericValueParamVFInt);

    for (Entry<Integer, String> e : usedValues.entrySet()) {
      String valueName = "A";
      JEnumConstant enumConstant = enumClass.enumConstant(valueName);
      switchVF._case(JExpr.lit(e.getKey())).body()._return(enumConstant);
    }

ie as u say, but with return instead break. also it calls enumConstant() twice, plus its having argument.

@phax
Copy link
Owner

phax commented Aug 18, 2017

Okay, so you have a method where the case is based on an int and the return value is an enum constant. Working on a test case

@mushaorc
Copy link

correct, sorry for being unclear in the begining.

@phax
Copy link
Owner

phax commented Aug 18, 2017

Test code:

    final JCodeModel cm = new JCodeModel ();

    final JDefinedClass c2 = cm._package ("issue41")._class ("Issue41Test2");

    final JDefinedClass jEnumClass = c2._enum ("MyEnum");
    final JEnumConstant ca = jEnumClass.enumConstant ("A");
    final JEnumConstant cb = jEnumClass.enumConstant ("B");
    jEnumClass.enumConstant ("C");

    final JMethod m = c2.method (0, jEnumClass, "dummy");
    final JVar p = m.param (cm.INT, "val");
    final JSwitch s = m.body ()._switch (p);
    s._case (JExpr.lit (1)).body ()._return (ca);
    s._case (JExpr.lit (2)).body ()._return (cb);
    s._default ().body ()._return (JExpr._null ());

output:

package issue41;

public class Issue41Test2 {

    Issue41Test2 .MyEnum dummy(int val) {
        switch (val) {
            case  1 :
            {
                return Issue41Test2 .MyEnum.A;
            }
            case  2 :
            {
                return Issue41Test2 .MyEnum.B;
            }
            default:
            {
                return null;
            }
        }
    }

    public enum MyEnum {
        A,
        B,
        C;
    }
}

phax added a commit that referenced this issue Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants