Add support for alignment of user defined types.

[gmlarumbe]

Hi,

This PR adds support for alignment of user defined types when running verilog-pretty-declarations.

Related issues: #308 #386 #920

To perform alignment two variables are used:

• verilog-typedef-regexp: already existing variable, string that matches user types.
  • In order to carry out alignment it is required to match the Verilog identifier regexp, e.g. "\\<[a-zA-Z_][a-zA-Z_0-9]*_t\\>" or (concat "\\<" verilog-identifier-re "_t\\>").
  • With a prefix/suffix format (e.g. "_t$") alignment is not supported but AUTO functions still work as they did before.
• verilog-typedef-words: new variable, list of strings that match user types, e.g. '("my_type" "custom_if" "my_enum"). Defaults to nil.

Thanks!

[wsnyder]

verilog-typedef-regexp: already existing variable, string that matches user types.
In order to carry out alignment it is required to match the Verilog identifier regexp, e.g. "\<[a-zA-Z_][a-zA-Z_0-9]*_t\>" or (concat "\<" verilog-identifier-re "_t\>").

We cannot require eval: in local variables, and verilog-typedef-regexp is extremely commonly used with the existing definition, so please use it as-is without adding the identifier regexp. (Just add the identifier regexp where used or in a function that merges it with typedef regexp)

verilog-typedef-words: new variable, list of strings that match user types, e.g. '("my_type" "custom_if" "my_enum"). Defaults to nil.

Doesn't verilog-typedef-regexp allow the exact same thing? Also generally most organizations I know use a naming convention and maintaining a specific list would be very painful.

[gmlarumbe]

Hi @wsnyder , thanks for the review,

[...] and verilog-typedef-regexp is extremely commonly used with the existing definition, so please use it as-is without adding the identifier regexp. (Just add the identifier regexp where used or in a function that merges it with typedef regexp)

Things will work as they did before with the existing definition. The only place where verilog-typedef-regexp is used is inside verilog-typedef-name-p:

(defun verilog-typedef-name-p (variable-name)
  "Return true if the VARIABLE-NAME is a type definition."
  (when verilog-typedef-regexp
    (verilog-string-match-fold verilog-typedef-regexp variable-name)))

Any valid type identifier ending in _t will return non-nil for both verilog-typedef-regexp being "_t$" or "\<[a-zA-Z_][a-zA-Z_0-9]*_t\>". However it is not possible to perform alignment with the "_t$" form since it is intended to match a substring of the type through verilog-string-match and not the whole type regexp through verilog-re-search-forward.

(defun verilog-get-declaration-typedef-re ()
  "Return regexp of a user defined typedef.
See `verilog-typedef-regexp' and `verilog-typedef-words'."
  (let (typedef-re words words-re re)
    (when (verilog-align-typedef-enabled-p)
      (setq typedef-re verilog-typedef-regexp)
      (setq words verilog-typedef-words)
      (setq words-re (verilog-regexp-words verilog-typedef-words))
      (cond ((and typedef-re (not words))
             (setq re typedef-re))
            ((and (not typedef-re) words)
             (setq re words-re))
            ((and typedef-re words)
             (setq re (concat verilog-typedef-regexp "\\|" words-re))))
      (concat "\\s-*" "\\(" verilog-declaration-prefix-re "\\s-*\\(" verilog-range-re "\\)?" "\\s-*\\)?"
              (concat "\\(" re "\\)")
              "\\(\\s-*" verilog-range-re "\\)?\\s-+"))))

I tried to figure out a way of obtaining the whole regexp for declaration matching from existing definition but I do not think its simple (i.e. obtaining "\<[a-zA-Z_][a-zA-Z_0-9]*_t\>" from "_t$" or "\<t_[a-zA-Z_][a-zA-Z_0-9]*\>" from "^t_"). The simplest way to achieve proper alignment is by setting the whole regexp of the type as part of the complete regexp to be searched for.

In 999498e I did the same but adding the variable verilog-align-typedef-regexp to avoid potential conflicts with verilog-typedef-regexp. I thought that in the end it would be redundant to have two variables to define the same thing (one for AUTOs and another for alignment) but if you think this is a better approach I can force-push it.

We cannot require eval: in local variables,

Why? It seems to work fine. Anyway, any line containing:

       // eval: (setq verilog-typedef-regexp (concat "\\<" verilog-identifier-re "_t\\>"))

Can be rewritten as:

        // verilog-typedef-regexp: "\\<[a-zA-Z_][a-zA-Z_0-9]*_t\\>"

Doesn't verilog-typedef-regexp allow the exact same thing? Also generally most organizations I know use a naming convention and maintaining a specific list would be very painful.

This is the purpose of current PR: any type matching verilog-typedef-regexp will be aligned in addition to any word belonging to verilog-typedef-words (for the case of other types that might not stick to naming conventions for any reason).

Thanks!

[gmlarumbe]

Current force-pushed commit replaces verilog-typedef-regexp and verilog-typedef-words with verilog-align-typedef-regexp and verilog-align-typedef-words and removes eval: from local variables.

[thomase00]

How is this intended to work? I have attached a test case align_test.v

I want mystruct_t to be recognized as part of the type and treated for the purposes of alignment the same way as "logic" is treated in "output logic out;". I thought this is what verilog-align-typedef-regexp was added to handle, but it doesn't work as I expect. If I instead add the whole type name "mystruct_t" to the verilog-align-typedef-words list, it skips alignment entirely for that line.

align_test.txt

[gmlarumbe]

Hi @thomase00 ,

You can check #1823 (comment).

For your particular case:

• You can set the variable verilog-align-typedef-regexp to the Emacs Lisp regexp of the whole Verilog identifier:

  // Local Variables:
  // eval: (setq verilog-align-typedef-regexp (concat "\\<" verilog-identifier-re "_\\(t\\)\\>"))
  // End:

• Or you can add the string "mystruct_t" to the verilog-align-typedef-words list:

  // Local Variables:
  // verilog-align-typedef-words: ("mystruct_t")
  // End:

I tried both locally and worked fine with the latest verilog-mode version.

[thomase00]

This causes it to not perform any alignment at all for declarations of the type "mystruct_t". Is this as intended? The syntax highlighting (not shown in my align_test.txt file) seems to to still be recognizing "mystruct_t" as a port name rather than as part of the type. The actual port name "in" has no highlighting at all, unlike the other port names.

I was expecting the port name "in" to be aligned with the other port names, but the alignment of the type name "mystruct_t" to be left alone.

[thomase00]

Is the type name even really needed to do this correctly? Why not just have the alignment be anchored to the begging of the last identifier or the begging of the first of a list of comma-delimited identifiers at the end of the line.

[gmlarumbe]

This causes it to not perform any alignment at all for declarations of the type "mystruct_t".

I was expecting the port name "in" to be aligned with the other port names, but the alignment of the type name "mystruct_t" to be left alone.

I am not sure what is the result you get after running verilog-pretty-declarations. This is what I get:

The syntax highlighting (not shown in my align_test.txt file) seems to to still be recognizing "mystruct_t" as a port name rather than as part of the type. The actual port name "in" has no highlighting at all, unlike the other port names.

Syntax highlighting of variables through verilog-fontify-variables is not related to declaration alignment in verilog-mode. If you want to fontify user defined types you can use the verilog-ext package, as pointed in #1823 (comment).

[thomase00]

Thanks! I've never used verilog-pretty-declarations before, I've only ever used TAB while editing on-the-fly.

I'm guessing this a workaround to avoid doing a pass over the buffer after every line, due to there not being a good way to maintain the "state" of indentation and alignment on-the-fly...