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

Auto-comment and indentation improvements #2

Closed
wants to merge 0 commits into from

Conversation

acr4
Copy link
Contributor

@acr4 acr4 commented Oct 27, 2014

Please consider the commits in this pull-request to enhance behavior of auto-comment and indentation of various Verilog and SV constructs.

@wsnyder
Copy link
Member

wsnyder commented Oct 27, 2014

I'd prefer to track changes on veripool.org to keep things contained.

Anyhow first four are merged to trunk.

The last one I'm not sure of. I'm curious what is the case that is breaking with this, and also why only include is being fixed and none of the other preproc directives that have arguments?

Thanks for the work here, it's needed some love. I'd caution that we don't have any regression tests on the autocomment code; it would be great to have an improvement to 0test.el that updated the endcomments on every test, so we could regress this autocomment code.

@acr4
Copy link
Contributor Author

acr4 commented Oct 27, 2014

Thanks for accepting the first four. I originally tested the fifth using a
complete "ifdef ...endif" block before the property, but apparently
failed to test the define ... case or having the property inside the ifdef. Those indeed fail to correct the indentation.

Let me think a little more and get back to you on these...

On Mon, Oct 27, 2014 at 1:55 PM, W Snyder notifications@github.com wrote:

I'd prefer to track changes on veripool.org to keep things contained.

Anyhow first four are merged to trunk.

The last one I'm not sure of. I'm curious what is the case that is
breaking with this, and also why only include is being fixed and none of
the other preproc directives that have arguments?

Thanks for the work here, it's needed some love. I'd caution that we don't
have any regression tests on the autocomment code; it would be great to
have an improvement to 0test.el that updated the endcomments on every test,
so we could regress this autocomment code.


Reply to this email directly or view it on GitHub
#2 (comment).

@acr4 acr4 force-pushed the master branch 2 times, most recently from 13fd50d to 4badec7 Compare October 28, 2014 14:56
@acr4
Copy link
Contributor Author

acr4 commented Oct 28, 2014

Hi Wilson,
Here's another take on fixing property indentation after preproc directives:

acr4@906ae28
<edit: new commit via force-push to clean-up my terrible spelling>

Still no unit tests, but all of the IEEE 1800-2012 directives are accounted
for (the ones listed in Section 22)

On Mon, Oct 27, 2014 at 2:26 PM, Alex C. Reed, IV acreed4@gmail.com wrote:

Thanks for accepting the first four. I originally tested the fifth using
a complete "ifdef ...endif" block before the property, but apparently
failed to test the define ... case or having the property inside the ifdef. Those indeed fail to correct the indentation.

Let me think a little more and get back to you on these...

On Mon, Oct 27, 2014 at 1:55 PM, W Snyder notifications@github.com
wrote:

I'd prefer to track changes on veripool.org to keep things contained.

Anyhow first four are merged to trunk.

The last one I'm not sure of. I'm curious what is the case that is
breaking with this, and also why only include is being fixed and none of
the other preproc directives that have arguments?

Thanks for the work here, it's needed some love. I'd caution that we
don't have any regression tests on the autocomment code; it would be great
to have an improvement to 0test.el that updated the endcomments on every
test, so we could regress this autocomment code.


Reply to this email directly or view it on GitHub
#2 (comment).

@wsnyder
Copy link
Member

wsnyder commented Nov 3, 2014

Committed, just made small change as `begin_keywords needs to support quotes. Thanks.

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.

None yet

2 participants