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

Wrong indentation with single-line case statements #51

Closed
xguerin opened this issue Feb 3, 2016 · 23 comments
Closed

Wrong indentation with single-line case statements #51

xguerin opened this issue Feb 3, 2016 · 23 comments
Assignees
Labels
Milestone

Comments

@xguerin
Copy link

xguerin commented Feb 3, 2016

In the following situation:

case (Signal)
  2'd0: begin Result <= 0; end
  2'd1: begin Result <= 1; end
  2'd2: begin Result <= 2; end
  default: begin Result <= 0; end
endcase

the automatic indenting gives the result below:

case (Signal)
  2'd0: begin Result <= 0; end
    2'd1: begin Result <= 1; end
      2'd2: begin Result <= 2; end
        default: begin Result <= 0; end
endcase
@lewis6991
Copy link
Collaborator

Its because the begin and end are on the same line. Is there any reason you are writing the case statement in this style? The begin and end's are redundant.

@xguerin
Copy link
Author

xguerin commented Feb 3, 2016

Yes, it actually makes register mapping a bit more readable. Another type of issue arises when I skip begin and end:

case (XfrState)
  One_XFR : XfrState_str = "One";
  Two_XFR : XfrState_str = "Two";
  End_XFR : XfrState_str = "End";
  default : XfrState_str = "N/A";
endcase

is formatted :

case (XfrState)
  One_XFR : XfrState_str = "One";
  Two_XFR : XfrState_str = "Two";
  End_XFR : XfrState_str = "End";
default : XfrState_str = "N/A";
endcase

or, within a process:

always @(XfrState)
  case (XfrState)
    One_XFR : XfrState_str = "One";
    Two_XFR : XfrState_str = "Two";
    End_XFR : XfrState_str = "End";
default : XfrState_str = "N/A";
  endcase
end

@lewis6991
Copy link
Collaborator

I'm still not convinced about the begin statement(); end style. Im my opinion it should be avoided and discouraged. An older version of the new script did support this style, however I removed this at the benefit of better performance. In the future I will review whether adding this back effects performance and If it doesn't I'll change it back.

The other issue on the other hand was a different problem because the script wasn't reliably case-sensitive. I've just pushed a fix. Thanks for raising this.

@xguerin
Copy link
Author

xguerin commented Feb 3, 2016

I don't mind adapting the code. Thanks for looking into it.

@vhda
Copy link
Owner

vhda commented Feb 15, 2016

@lewis6991 I am unable to replicate the second situation in my current master branch. You did not include the case-sensitive fix in the PR, correct?

@lewis6991
Copy link
Collaborator

That is correct.

@lewis6991
Copy link
Collaborator

The reason you cannot replicate is because you probably don't have set ignorecase in your vimrc.

Having this set changes the behaviour of the plugin. My commit for this was to change this option and restore it at the end of indenting.

@vhda
Copy link
Owner

vhda commented Feb 15, 2016

Understood. Let me think about this for some a day or two.

@lewis6991
Copy link
Collaborator

The only other solution I can think of is to have \C in every regex which I think would be a bit of a pain.

@vhda
Copy link
Owner

vhda commented Feb 15, 2016

That's not the problem.

Try the exact same test case as above, but replace "End" with "end". I'm almost 100% sure that it will be incorrectly indented even with you case-insensitive fix.

We need to filter out strings. Maybe by doing something like we're doing for comments: we just strip them out from the input before parsing?

@vhda
Copy link
Owner

vhda commented Feb 15, 2016

Just submitted some more changes. Please check them.

Could you take a look at the "end" issue for me? I can't invest more time in this for today :)

@lewis6991
Copy link
Collaborator

Stripping out strings may be the best solution. I'm just a little worried about performance.

@vhda vhda added the bug label Feb 15, 2016
@vhda vhda assigned vhda and unassigned vhda Feb 15, 2016
@lewis6991
Copy link
Collaborator

Feel free to assign to me. I'll check it out when I get some time.

@vhda
Copy link
Owner

vhda commented Feb 15, 2016

I was trying to!!! For some reason you don't appear in the list!

@lewis6991
Copy link
Collaborator

Is it because it is your personal repo and not an organisation?

@vhda
Copy link
Owner

vhda commented Feb 15, 2016

Let me try this: @lewis6991

@vhda
Copy link
Owner

vhda commented Feb 15, 2016

Dumb me: https://help.github.com/articles/assigning-issues-and-pull-requests-to-other-github-users/

"You can only create assignments for yourself, collaborators on personal projects, or members of your organization with read permissions on the repository."

@vhda vhda added this to the Release 3.0 milestone Feb 16, 2016
@lewis6991
Copy link
Collaborator

Fixed in 7d83a3e

@vhda
Copy link
Owner

vhda commented Feb 18, 2016

Nice! Thanks.

@lewis6991
Copy link
Collaborator

Hope you don't mind me making commits like this without PR'ing them.

@vhda
Copy link
Owner

vhda commented Feb 18, 2016

I don't mind, but I don't think that's the best workflow.

Now that I have a collaborator I'm considering starting to PR my own commits, giving us the opportunity to review and discuss the changes before submitting. Hopefully, this will result in better and more feature complete code.

@lewis6991
Copy link
Collaborator

I agree. I only pushed the commits today as I felt they were fairly trivial and small. I'll put future ones in a PR.

@vhda
Copy link
Owner

vhda commented Feb 18, 2016

Will do the same.

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

3 participants