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

Module mis-indent within generate statement #1163

Closed
veripoolbot opened this issue May 16, 2017 · 12 comments
Closed

Module mis-indent within generate statement #1163

veripoolbot opened this issue May 16, 2017 · 12 comments
Labels

Comments

@veripoolbot
Copy link
Collaborator


Author Name: Brad Dobbie
Original Redmine Issue: 1163 from https://www.veripool.org


I've encountered an indentation problem, shown below. verilog-mode-version is "2017-04-28-6b4fc78-vpo". I'm pretty sure this problem wasn't present in previous versions, but I can't say for sure.

Notice the opening parenthesis of the instance port connections is indented way too far. If I remove the "=0" on the generate for (...), the indentation works correctly (but obviously that isn't valid syntax).

module indent;

    // Here, AUTOINST line indents incorrectly indents way too far the the right
    generate for (genvar i=0; i<4; i++) begin : gen_inst0
       subindent s
                           (/*AUTOINST*/
                            // Outputs
                            .y                           (y),
                            // Inputs
                            .a                           (a));
    end endgenerate

    // Without the '=0', the AUTOINST line indents properly
    generate for (genvar i; i<4; i++) begin : gen_inst1
       subindent s
         (/*AUTOINST*/
          // Outputs
          .y                             (y),
          // Inputs
          .a                             (a));
    end endgenerate

endmodule

module subindent (input a, output y);
endmodule

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-05-16T17:55:20Z


FYI I looked at old revisions and didn't find any that work with the last 2 years.

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Kaushal Modi
Original Date: 2017-05-16T18:40:52Z


I confirm this issue too.

I never saw this issue earlier though as my coding style is a bit different.

Below indents fine.

module indent;

    genvar i;
    generate
       for (i=0; i<4; i++) begin : gen_inst0
          subindent s
              (/*AUTOINST*/
               // Outputs
               .y                           (y),
               // Inputs
               .a                           (a));
       end 
    endgenerate

endmodule

module subindent (input a, output y);
endmodule

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-05-16T18:53:25Z


Test committed as indent_genmod.v. Kaushal, perhaps you could take a look when you get a chance?

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Brad Dobbie
Original Date: 2017-05-22T16:03:49Z


Kaushal Modi wrote:

I confirm this issue too.

I never saw this issue earlier though as my coding style is a bit different.

Below indents fine.
[...]

Kashul, the issue is still present in your coding style but it is harder to see. If I space out the for statement, you can see the open parenthesis getting aligned with the = sign.

    genvar i;
    generate
       for                          (i=0; i<4; i++) begin : gen_inst0
          subindent s
                                       (/*AUTOINST*/
                                        // Outputs
                                        .y                           (y),
                                        // Inputs
                                        .a                           (a));
       end 
    endgenerate

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Kaushal Modi
Original Date: 2017-05-22T17:42:32Z


@wilson: I cannot promise when, but I am putting this on my list to investigate. My long term goal is to document all the regexp defvars. That exercise should help fix issues like these :)

@brad: You're right. I'll have a look at this when I get chance. Debugging indentation logic is a bit involved.

@ruelparent
Copy link

ruelparent commented Oct 17, 2020

I took a look at this because it was bugging me.

(defconst verilog-extended-complete-re
  ;; verilog-beg-of-statement also looks backward one token to extend this match
  (concat "\\(\\(\\<extern\\s-+\\|\\<\\(\\<\\(pure\\|context\\)\\>\\s-+\\)?virtual\\s-+\\|\\<protected\\s-+\\|\\<static\\s-+\\)*\\(\\<function\\>\\|\\<task\\>\\)\\)"
	  "\\|\\(\\(\\<typedef\\>\\s-+\\)*\\(\\<struct\\>\\|\\<union\\>\\|\\<class\\>\\)\\)"
	  "\\|\\(\\(\\<\\(import\\|export\\)\\>\\s-+\\)?\\(\"DPI\\(-C\\)?\"\\s-+\\)?\\(\\<\\(pure\\|context\\)\\>\\s-+\\)?\\([A-Za-z_][A-Za-z0-9_]*\\s-*=\\s-*\\)?\\(function\\>\\|task\\>\\)\\)"
	  "\\|" verilog-extended-case-re ))

the term of interest appears to be "\([A-Za-z_][A-Za-z0-9_]\s-=\s-*\)?" it matches the genvar assignment in the for loop "i=0". the indention after that point is aligned with the value so where ever the '=' sign is the key.

here is the stack trace:

Debugger entered--returning value: t
looking-at("\\(\\(\\<extern\\s-+\\|\\<\\(\\<\\(pure\\|context\\)\\>\\s-+\\)?virtual\\s-+\\|\\<protected\\s-+\\|\\<static\\s-+\\)*\\(\\<function\\>\\|\\<task\\>\\)\\)\\|\\(\\(\\<typedef\\>\\s-+\\)*\\(\\<struct\\>\\|\\<union\\>\\|\\<class\\>\\)\\)\\|\\(\\(\\<\\(import\\|export\\)\\>\\s-+\\)?\\(\"DPI\\(-C\\)?\"\\s-+\\)?\\(\\<\\(pure\\|context\\)\\>\\s-+\\)?\\([A-Za-z_][A-Za-z0-9_]*\\s-*=\\s-*\\)?\\(function\\>\\|task\\>\\)\\)\\|\\(\\(unique0?\\s-+\\|priority\\s-+\\)?case[xz]?\\|randcase\\)\\|\\(\\<\\(a\\(?:lways\\(?:_\\(?:comb\\|ff\\|latch\\)\\)?\\|ss\\(?:ert\\|ign\\)\\)\\|con\\(?:nectmodule\\|straint\\)\\|do\\|else\\|f\\(?:inal\\|or\\(?:e\\(?:ach\\|ver\\)\\)?\\)\\|i\\(?:f\\|mport\\|nitial\\)\\|localparam\\|m\\(?:\\(?:acrom\\)?odule\\)\\|parameter\\|r\\(?:andcase\\|epeat\\)\\|while\\)\\>\\)")`
* byte-code("\303!\204)
* verilog-beg-of-statement-1()
* #[(indent-str) "@A@\211\306=\203'`\307 \210h\310U\204#
* funcall(#[(indent-str) "@A@\211\306=\203'`\307 \210h\310U\204#
* verilog-do-indent((cexp 6))
  verilog-indent-line()

I don't know the purpose of that term in the regexp. so not sure what the next steps should be.

Update, I removed that expression and it did not change the indent level.

(defun verilog-do-indent (indent-str)
  (let ((type (car indent-str))
	(ind (car (cdr indent-str))))
    (cond
     (; handle continued exp
      (eq type 'cexp)

......

	  (let ((val))
	    (verilog-beg-of-statement-1)
	    (if (and (< (point) here)
		     (verilog-re-search-forward "=[ \t]*" here 'move)
		     ;; not at a |=>, #=#, or [=n] operator
		     (not (string-match "\\[=.\\|#=#\\||=>"
                                        (or (buffer-substring (- (point) 2) (1+ (point)))
                                            ""))))  ; don't let buffer over/under-run spoil the party
		(setq val (current-column))
	      (setq val (eval (cdr (assoc type verilog-indent-alist)))))
	    (goto-char here)
	    (indent-line-to val))))))

it caught the "for" term and then searched forward for an '='.

I tried putting something, i used an always_comb, before the instance and that fixed the indenting of the instance port list. Is there a way to check for an "instance" in the search for beginning of statement?

@ruelparent
Copy link

This code works for the scenarios I tried. I have never contributed before not sure the process:

	 (t
	  (goto-char here)
	  (let ((val))
            ;; #1163 - New Code Look if it is a generated instance.  The problem is the line which has the format like:
            ;;  for(genvar i=0; i<2; i=i+1) begin
            ;;    mod_1  inst_1
            ;;      (/*AUTOINST*/); << this line will indent wrong with old code
            (if (looking-at ".*#?(.*$")
                (setq val (eval (cdr (assoc type verilog-indent-alist))))
              (progn 
                ;; end of new code
                (verilog-beg-of-statement-1)
                (if (and (< (point) here)
                         (verilog-re-search-forward "=[ \t]*" here 'move)
                         ;; not at a |=>, #=#, or [=n] operator
                         (not (string-match "\\[=.\\|#=#\\||=>"
                                            (or (buffer-substring (- (point) 2) (1+ (point)))
                                                ""))))  ; don't let buffer over/under-run spoil the party
                    (setq val (current-column))
                  (setq val (eval (cdr (assoc type verilog-indent-alist)))))
                )) ;; New Code
            (goto-char here)
            (indent-line-to val))))))

@wsnyder
Copy link
Member

wsnyder commented Oct 18, 2020

Thanks for looking at improving the indents.

Unfortunately there's no longer anyone super familiar with the indent code to provide insights as to the past.

This code seems to be looking to the =, which doesn't seem right, I would think that if the "mod" is properly being indented the next line should follow with appropriate relative indent.

As to contributing, it's best to make a pull request, search for "github pull request tutorial" or similar - they'll describe the process better than I would ;)

@acr4
Copy link
Contributor

acr4 commented Oct 18, 2020 via email

ruelparent added a commit to ruelparent/verilog-mode that referenced this issue Oct 25, 2020
@ruelparent
Copy link

I created a pull request.

@wsnyder
Copy link
Member

wsnyder commented Oct 26, 2020

Looking backwards like this seems overly sensitive note one of the regression tests is now failing. The traditional approach is to look forward adjusting the indent appropriately, but as noted I haven't been in this code much perhaps @acr4 can comment.

@ruelparent
Copy link

I think it does the backward search since it is classified as a continuing expression. it is search backward to determine the exact nature of the expression.

wsnyder added a commit that referenced this issue Sep 16, 2021
* verilog-mode.el (verilog-in-generate-region-p):
Fix instance port indention inside generate for loop (#1163) (#1699).
Reported by ruelparent.
@wsnyder wsnyder closed this as completed Sep 16, 2021
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