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

indenting for some forms of SystemVerilog constraints is wrong/odd #433

Closed
veripoolbot opened this issue Jan 27, 2012 · 9 comments
Closed
Labels

Comments

@veripoolbot
Copy link
Collaborator


Author Name: Brad Parker (@lisper)
Original Redmine Issue: 433 from https://www.veripool.org
Original Date: 2012-01-27
Original Assignee: Alex Reed


indentation for several different forms of SV constraints is wrong or odd;
specifically, empty constraints are odd, as are constraints with "if" followed by braces

It acts like it doesn't understand the braces (i.e. {})

Here's an example:

class data;
rand integer data1;
rand integer data2,data3;
rand reg [31:0] foo;

 constraint basic_c {
    
    //empty constraint
		}
   
   constraint complex_c {
		    data1 <= 100;
		    foo inside {[40:999]};
		    if(foo < 87)
		    data2 == 10;
		    if(data2 == 76) {
 data3 == 8;
   }
		    }

constraint implication_c {
   data1 == 10 -> data3 >= -1;
}
 
function new();
   data1 = 0;
   data2 = 78;
endfunction // new

endclass // data

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Brad Parker (@lisper)
Original Date: 2012-01-27T22:52:26Z


wow. the html (or php?) code "displayer" really mangled that code. That's not at all what I pasted.

I'll try and with a pre block...

class data;
     rand integer data1;
     rand integer data2,data3;
     rand reg [31:0] foo;
     
     constraint basic_c {
        
        //empty constraint
			}
       
       constraint complex_c {
			    data1 <= 100;
			    foo inside {[40:999]};
			    if(foo < 87)
			    data2 == 10;
			    if(data2 == 76) {
	 data3 == 8;
       }
			    }
	
	constraint implication_c {
	   data1 == 10 -> data3 >= -1;
	}
     
	function new();
	   data1 = 0;
	   data2 = 78;
	endfunction // new    
 endclass // data

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Brad Parker (@lisper)
Original Date: 2012-04-03T15:22:53Z


I made a small set of changes which solve "my problem" (as in, "works for me!" :-) with systemverilog constraints.
I now get indenting which looks rational...

I'm not 100% happy with the changes, but they are at least minimal and clean.
These diffs are against 735

--- verilog-mode.el.orig        2012-04-02 11:28:24.530560000 -0700
+++ verilog-mode.el     2012-04-02 13:50:26.922656000 -0700
@@ -5315,7 +5315,13 @@
           ((eq type 'defun)
            (list type 0))
           (t
-          (list type (verilog-current-indent-level))))))))
+          ;;(message "block point %s starting_position %s indent %s inside-constraint %s"
+          ;;       (point) starting_position
+          ;;       (verilog-current-indent-level-pos starting_position)
+          ;;       (verilog-inside-constraint-pos-p starting_position))
+          (if (verilog-inside-constraint-pos-p starting_position)
+              (list type (verilog-current-indent-level-pos starting_position))
+            (list type (verilog-current-indent-level)))))))))
 
 (defun verilog-wai ()
    "Show matching nesting block for debugging."
@@ -5921,6 +5927,7 @@
          (verilog-at-constraint-p)
          )
       nil)))
+
 (defun verilog-at-close-constraint-p ()
    "If at the } that closes a constraint or covergroup, return true."
    (if (and
@@ -5929,7 +5936,8 @@
 
        (save-excursion
         (verilog-backward-ws&directives)
-       (if (equal (char-before) ?\;)
+;;     (if (equal (char-before) ?\;)
+       (if (or (equal (char-before) ?\;) (equal (char-before) ?\}))
             (point)
           nil))))
 
@@ -5941,12 +5949,26 @@
          (forward-list)
          (progn (backward-char 1)
                 (verilog-backward-ws&directives)
-               (equal (char-before) ?\;))))
+;;             (equal (char-before) ?\;))))
+               (or (equal (char-before) ?\;) (equal (char-before) ?\})))))
        ;; maybe
        (verilog-re-search-backward "\\<constraint\\|coverpoint\\|cross\\>" nil 'move)
      ;; not
      nil))
 
+(defun verilog-inside-constraint-pos-p (pos)
+  "If inside a constraint or coverpoint definition, return true, moving point to constraint."
+  (save-excursion
+    (let (par-pos)
+      (beginning-of-line)
+      (setq par-pos (verilog-parenthesis-depth))
+      (while par-pos
+       (goto-char par-pos)
+       (beginning-of-line)
+       (setq par-pos (verilog-parenthesis-depth)))
+      (skip-chars-forward " \t")
+      (verilog-re-search-forward "\\<constraint\\>" pos 'move))))
+
 (defun verilog-at-struct-p ()
    "If at the { of a struct, return true, moving point to struct."
    (save-excursion
@@ -6227,6 +6249,21 @@
      indent-str                         ; Return indent data
      ))
 
+(defun verilog-current-indent-level-pos (pos)
+  "Return the indent-level of the current statement."
+  (save-excursion
+    (let (par-pos)
+      (goto-char pos)
+      (beginning-of-line)
+      (setq par-pos (verilog-parenthesis-depth))
+      (if par-pos
+         (progn
+           (goto-char par-pos)
+           (beginning-of-line)
+           (skip-chars-forward " \t")
+           (current-column))
+       0))))
+
 (defun verilog-current-indent-level ()
    "Return the indent-level of the current statement."
    (save-excursion



@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2012-04-09T19:03:04Z


As you said, this seems better, but is not perfect. Even so I was ready to commit it, however it breaks basic Verilog indentation with the following, so unfortunately I can't merge it. Attached is a modified version of your patch that just changes a few naming and related issues if you'd like to fix that one instead.

  always_comb
      begin
         case (in1)
           4'b0001 :     begin
              out1  = in2;
           end
           default       :       begin
              out1  = {4{1'b0}};
           end  // This mis-indents
         endcase
      end

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Brad Parker (@lisper)
Original Date: 2012-04-10T11:32:12Z


I noticed that also (but only yesterday). I'll fix that and resubmit. thanks!

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Austin Harris
Original Date: 2013-02-21T07:45:49Z


Can you please assign someone to fix this? Any OVM/UVM verification code is extremely annoying to deal with because of this.

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-02-21T12:27:59Z


The way things get fixed with open source is you "assign" yourself to fix it.

Bruce bluewlvrn@gmail.com submitted another patch (attached), but this still breaks basic indents so cannot be merged. Perhaps you could ask if he needs assistance getting this ready.

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Austin Harris
Original Date: 2013-07-03T18:38:11Z


Hello, the constraints indenting is working great now in most cases, however I am seeing an issue with the following snippet:

if (x)
  `ovm_do_with(my,
                { y == 1;
    z == 2;
});

Could anyone give me any guidance on where to look to go about fixing this?

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Alex Reed
Original Date: 2015-03-04T17:51:15Z


Wilson,
Please merge https://github.com/acr4/verilog-mode/commits/bugfix-433 which fixes the supplied test-case. emacs and xemacs regressions pass (for me anyway...).
Thanks,
-Alex

@veripoolbot
Copy link
Collaborator Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-03-04T21:21:16Z


Thanks again, pushed to git.

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

1 participant