Add (optional) rule do forbid inline comments

[staerz]

Is your feature request related to a problem? Please describe. It is general purpose and overview esthetics: Inline comments can be overlooked easily.

Describe the solution you'd like As there is rule comment_004 which checks for inline comments, I guess it would be easy to add a rule that entirely forbids inline comments.

Maybe that rule could even self-fix by making the inline comment a regular comment (in the line above with the correct indentation).

As that is an optional rule, it should be disabled by default.

**Describe alternatives you've considereA clear and concise description of any alternative solutions or features you've considered. There is no real alternative (other then writing an own rule on that).

Additional context Inline comments may be hidden and overlooked easily.

[jeremiah-c-leary]

Hey @staerz ,

So this new rule would turn this:

   a <= b;  -- Some comment

into this:

  -- Some comment
  a <= b;

[jeremiah-c-leary]

Hey @staerz,

I push an update with a new rule "comment_011" to handle this. When you get a chance, could you validate it on your end.

Thanks,

--Jeremy

[staerz]

Hi @jeremiah-c-leary,

from what the documentation says I agree to the change.

When testing it, I run into this:

multiprocessing.pool.RemoteTraceback: 
"""
Traceback (most recent call last):
  File "/usr/lib64/python3.8/multiprocessing/pool.py", line 125, in worker
    result = (True, func(*args, **kwds))
  File "/usr/local/lib/python3.8/site-packages/vsg-3.0.0.dev10-py3.8.egg/vsg/__main__.py", line 469, in apply_rules
    oRules.check_rules(bAllPhases=commandLineArguments.all_phases, lSkipPhase=commandLineArguments.skip_phase)
  File "/usr/local/lib/python3.8/site-packages/vsg-3.0.0.dev10-py3.8.egg/vsg/rule_list.py", line 217, in check_rules
    oRule.analyze(self.oVhdlFile)
  File "/usr/local/lib/python3.8/site-packages/vsg-3.0.0.dev10-py3.8.egg/vsg/rule.py", line 131, in analyze
    self._analyze(lToi)
  File "/usr/local/lib/python3.8/site-packages/vsg-3.0.0.dev10-py3.8.egg/vsg/rules/comment/rule_011.py", line 27, in _analyze
    if utils.does_line_start_with_comment(lTokens):
  File "/usr/local/lib/python3.8/site-packages/vsg-3.0.0.dev10-py3.8.egg/vsg/rules/utils.py", line 42, in does_line_start_with_comment
    if isinstance(lTokens[0], parser.comment):
IndexError: list index out of range
"""

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/local/bin/vsg", line 11, in <module>
    load_entry_point('vsg==3.0.0.dev10', 'console_scripts', 'vsg')()
  File "/usr/local/lib/python3.8/site-packages/vsg-3.0.0.dev10-py3.8.egg/vsg/__main__.py", line 412, in main
    for tResult in pool.imap(f, enumerate(commandLineArguments.filename)):
  File "/usr/lib64/python3.8/multiprocessing/pool.py", line 868, in next
    raise value
IndexError: list index out of range

The change in our code base is that I added the comment here:

library fpga;
  context fpga.interfaces; -- here is an inline comment

[jeremiah-c-leary]

Hey @staerz ,

I used this example file:

  1
  2 library fpga;
  3   context fpga.interfaces; -- here is an inline comment
  4
  5 architecture rtl of fifo is
  6
  7 begin
  8
  9   a <= b; -- Comment
 10
 11 end architecture rtl;

and it did not crash. Could you try that snippet in your environment?

[staerz]

Hi @jeremiah-c-leary,

I could identify the problematic lines:

It is the very first lines of a file which are (by default in our setup) the following ones:

-- EMACS settings: -*- tab-width: 2; indent-tabs-mode: nil -*-
-- vim: tabstop=2:shiftwidth=2:expandtab
-- kate: tab-width 2; replace-tabs on; indent-width 2;

As you can see, these are indicators for certain file editors to get the tabbing right upon opening.

If I remove these, then VSG runs without that reported error, and running with the --fix option also fixes the line as needed, except that the indent is wrong - did you check it? I would guess that you apply the default indent as for any comment in that line, right? (But it looks like the default +-1 error :wink: )

[jeremiah-c-leary]

Hey @staerz , I updated my snippet to this:

  1 -- This should pass
  2
  3 library fpga;
  4   context fpga.interfaces; -- here is an inline comment
  5
  6 architecture rtl of fifo is
  7
  8 begin
  9
 10   a <= b; -- Comment
 11
 12 end architecture rtl;

If I add a return before the first comment it works. I know where the issue is.

[jeremiah-c-leary]

Hey @staerz ,

I just pushed a fix for this issue.

[staerz]

Hi @jeremiah-c-leary,

alright, now VSG doesn't crash anymore, but the indent for auto-fixing is still not correct. This is what I get:

library fpga;
 -- here is an inline comment
  context fpga.interfaces;

[jeremiah-c-leary]

Hey @staerz , That is odd. I modified the snippet like this:

  1 -- This should pass
  2
  3 library fpga;
  4   context fpga.interfaces;-- here is an inline comment
  5
  6 architecture rtl of fifo is
  7
  8 begin
  9
 10   a <= b; -- Comment
 11
 12 end architecture rtl;

Used this configuration file:

  1 rule:
  2   comment_011:
  3     disable : False

and the snippet was fixed to this:

  1 -- This should pass
  2
  3 library fpga;
  4   -- here is an inline comment
  5   context fpga.interfaces;
  6
  7 architecture rtl of fifo is
  8
  9 begin
 10
 11   -- Comment
 12   a <= b;
 13
 14 end architecture rtl;

Is there a possibility a configuration option is not allowing the indent of the comment?

[jeremiah-c-leary]

Hey @staerz,

Just wanted to ping you and see if you had a chance to look at my response.

--Jeremy

[staerz]

Well, also here, no updates, but maybe some more code examples would help:

         end case;

       end if;
-    end if; -- clk_sctrl_i
+ -- clk_sctrl_i
+    end if;

   end process proc_sctrl_test;

           qsfp1_sda      <= '0';
           qsfp1_scl      <= '0';
         else
-          rst_feb2_link  <= (others => '0'); --"0" & not user_pb;
+ --"0" & not user_pb;
+          rst_feb2_link  <= (others => '0');
           qsfp1_mod_seln <= '1';
           qsfp1_sda      <= 'Z';
           qsfp1_scl      <= 'Z';

         when 1 to 28 =>
           for i in 0 to DEPTH / 2 - 1 loop
             for j in 0 to 3 loop
-              uniform(seed1, seed2, rand(j));          -- generate random number
+          -- generate random number
+              uniform(seed1, seed2, rand(j));
             end loop;
-            ax(i) <= integer(rand(0) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
-            ay(i) <= integer(rand(1) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
-            bx(i) <= integer(rand(2) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
-            by(i) <= integer(rand(3) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
+ -- rescale to 0..1000, convert integer part
+            ax(i) <= integer(rand(0) * RANGE_OF_RAND);
+ -- rescale to 0..1000, convert integer part
+            ay(i) <= integer(rand(1) * RANGE_OF_RAND);
+ -- rescale to 0..1000, convert integer part
+            bx(i) <= integer(rand(2) * RANGE_OF_RAND);
+ -- rescale to 0..1000, convert integer part
+            by(i) <= integer(rand(3) * RANGE_OF_RAND);
           end loop;
           valid   <= '1';
         when others =>

Here it works:

   --! @todo make the range 'downto'
   constant C_WIENER     : integer_vector(0 to 5) :=
   (
-    -9379,  -- c(0)
-    17867,  -- c(1)
-    44585,  -- c(2)
-    -68495, -- c(3)
-    36820,  -- c(4)
-    -8483   -- c(5)
+    -- c(0)
+    -9379,
+    -- c(1)
+    17867,
+    -- c(2)
+    44585,
+    -- c(3)
+    -68495,
+    -- c(4)
+    36820,
+    -- c(5)
+    -8483
   );
   -- vsg_on whitespace_011

[jeremiah-c-leary]

Hey @staerz,

So I ran a couple of your examples and it seems to work for me. Which version of vsg are you running? I am on origin/master and the version is 3.0.0.dev29.

[staerz]

Hi @jeremiah-c-leary,

$ vsg --version
VHDL Style Guide (VSG) version: 3.0.0.dev29
Git commit SHA: ge2541a6a

so no clue what would be different - except maybe the VSG config (I don't remember in which issue I uploaded it ...)

[jeremiah-c-leary]

could you try this file:

  1 -- This should pass
  2
  3 library fpga;
  4 context fpga.interfaces;-- here is an inline comment
  5
  6 architecture rtl of fifo is
  7
  8    --! @todo make the range 'downto'
  9    constant C_WIENER     : integer_vector(0 to 5) :=
 10    (
 11     -9379,  -- c(0)
 12     17867,  -- c(1)
 13     44585,  -- c(2)
 14     -68495, -- c(3)
 15     36820,  -- c(4)
 16     -8483   -- c(5)
 17    );
 18
 19 begin
 20
 21   a <= b; -- Comment
 22
 23   PROC_LABEL : process begin
 24
 25     a <= b;-- Some Comment
 26
 27     case blah is
 28          when 1 to 28 =>
 29            for i in 0 to DEPTH / 2 - 1 loop
 30              for j in 0 to 3 loop
 31               uniform(seed1, seed2, rand(j));          -- generate random number
 32              end loop;
 33             ax(i) <= integer(rand(0) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
 34             ay(i) <= integer(rand(1) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
 35             bx(i) <= integer(rand(2) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
 36             by(i) <= integer(rand(3) * RANGE_OF_RAND); -- rescale to 0..1000, convert integer part
 37            end loop;
 38            valid   <= '1';
 39     end case;
 40
 41   end process;
 42
 43 end architecture rtl;

using this configuration:

  1 rule:
  2   comment_011:
  3     disable : False

When I issue: vsg -c config.yaml -f snippet.vhd --fix I get the following:

  1 -- This should pass
  2
  3 library fpga;
  4   -- here is an inline comment
  5   context fpga.interfaces;
  6
  7 architecture rtl of fifo is
  8
  9   --! @todo make the range 'downto'
 10   constant c_wiener : integer_vector(0 to 5) :=
 11   (
 12   -- c(0)
 13     -9379,
 14   -- c(1)
 15     17867,
 16   -- c(2)
 17     44585,
 18   -- c(3)
 19     -68495,
 20   -- c(4)
 21     36820,
 22   -- c(5)
 23     -8483
 24   );
 25
 26 begin
 27
 28   -- Comment
 29   a <= b;
 30
 31   proc_label : process is
 32   begin
 33
 34     -- Some Comment
 35     a <= b;
 36
 37     case blah is
 38
 39       when 1 to 28 =>
 40         for i in 0 to DEPTH / 2 - 1 loop
 41           for j in 0 to 3 loop
 42             -- generate random number
 43             uniform(seed1, seed2, rand(j));
 44           end loop;
 45           -- rescale to 0..1000, convert integer part
 46           ax(i) <= integer(rand(0) * RANGE_OF_RAND);
 47           -- rescale to 0..1000, convert integer part
 48           ay(i) <= integer(rand(1) * RANGE_OF_RAND);
 49           -- rescale to 0..1000, convert integer part
 50           bx(i) <= integer(rand(2) * RANGE_OF_RAND);
 51           -- rescale to 0..1000, convert integer part
 52           by(i) <= integer(rand(3) * RANGE_OF_RAND);
 53         end loop;
 54         valid   <= '1';
 55
 56     end case;
 57
 58   end process proc_label;
 59
 60 end architecture rtl;

Which makes sense to me because comment_011 is a phase 1 rule, and I implement indenting after phase 1 rules have been ran.

[staerz]

And I get this:

-- This should pass

library fpga;
-- here is an inline comment
  context fpga.interfaces;

architecture rtl of fifo is

   --! @todo make the range 'downto'
  constant C_WIENER : integer_vector(0 to 5) :=
  (
    -- c(0)
    -9379,
    -- c(1)
    17867,
    -- c(2)
    44585,
    -- c(3)
    -68495,
    -- c(4)
    36820,
    -- c(5)
    -8483
  );

begin

 -- Comment
  a <= b;

  proc_label : process
  begin

-- Some Comment
    a <= b;

    case blah is

      when 1 to 28 =>
        for i in 0 to DEPTH / 2 - 1 loop
          for j in 0 to 3 loop
          -- generate random number
            uniform(seed1, seed2, rand(j));
          end loop;
 -- rescale to 0..1000, convert integer part
          ax(i) <= integer(rand(0) * RANGE_OF_RAND);
 -- rescale to 0..1000, convert integer part
          ay(i) <= integer(rand(1) * RANGE_OF_RAND);
 -- rescale to 0..1000, convert integer part
          bx(i) <= integer(rand(2) * RANGE_OF_RAND);
 -- rescale to 0..1000, convert integer part
          by(i) <= integer(rand(3) * RANGE_OF_RAND);
        end loop;
        valid   <= '1';

    end case;

  end process proc_label;

end architecture rtl;

[jeremiah-c-leary]

That is bizarre. Which version of python are you running?

I have Python 3.8.2 and I am running on Ubuntu on WSL2.

[staerz]

Python 3.8.7 on a fedora 32

[staerz]

Would you see any configuration of another rule that could lead to different outcome?

I wouldn't expect so as we're talking about a new rule, but apparently the indent is different ...

[jeremiah-c-leary]

If I disable comment_10, I get the same thing you have:

  1 -- This should pass
  2
  3 library fpga;
  4 -- here is an inline comment
  5   context fpga.interfaces;
  6
  7 -- COmment
  8 architecture empty of fifo is
  9
 10    --! @todo make the range 'downto'
 11   constant C_WIENER : integer_vector(0 to 5) :=
 12   (
 13     -- c(0)
 14     -9379,
 15     -- c(1)
 16     17867,
 17     -- c(2)
 18     44585,
 19     -- c(3)
 20     -68495,
 21     -- c(4)
 22     36820,
 23     -- c(5)
 24     -8483
 25   );
 26
 27 begin
 28
 29  -- Comment
 30   a <= b;
 31
 32   proc_label : process
 33   begin
 34
 35 -- Some Comment
 36     a <= b;
 37
 38     case blah is
 39
 40       when 1 to 28 =>
 41         for i in 0 to DEPTH / 2 - 1 loop
 42           for j in 0 to 3 loop
 43           -- generate random number
 44             uniform(seed1, seed2, rand(j));
 45           end loop;
 46  -- rescale to 0..1000, convert integer part
 47           ax(i) <= integer(rand(0) * RANGE_OF_RAND);
 48  -- rescale to 0..1000, convert integer part
 49           ay(i) <= integer(rand(1) * RANGE_OF_RAND);
 50  -- rescale to 0..1000, convert integer part
 51           bx(i) <= integer(rand(2) * RANGE_OF_RAND);
 52  -- rescale to 0..1000, convert integer part
 53           by(i) <= integer(rand(3) * RANGE_OF_RAND);
 54         end loop;
 55         valid   <= '1';
 56
 57     end case;
 58
 59   end process proc_label;
 60
 61 end architecture rtl;

Could you check if comment_010 is disabled?

[staerz]

Hi @jeremiah-c-leary,

yes, I have comment_010 disabled as it's doing fishy things (that's why I opened #542), but it actually explains what's going on here.

One note, also on comment_010 would be the comment inside the constant declaration: It should take into account the opening parenthesis, shouldn't it?

But agreed, that's a matter of another issue, the initial claim of adding a rule that allows to complain on inline comments is treated and I think it's fair to finally close that one. Sorry for the long confusion.

[jeremiah-c-leary]

Hey @staerz,

One note, also on comment_010 would be the comment inside the constant declaration: It should take into account the opening parenthesis, shouldn't it?

Are you thinking it should look like this:

 11   constant C_WIENER : integer_vector(0 to 5) :=
 12   (
 13   -- c(0)
 14     -9379,
 15   -- c(1)
 16     17867,
 17   -- c(2)
 18     44585,
 19   -- c(3)
 20     -68495,
 21   -- c(4)
 22     36820,
 23   -- c(5)
 24     -8483
 25   );

where it is aligned with the parenthesis?

[staerz]

Hi @jeremiah-c-leary,

honestly, I don't remember anymore. I just read the thread again and maybe it was that you first had this:

  9   --! @todo make the range 'downto'
 10   constant c_wiener : integer_vector(0 to 5) :=
 11   (
 12   -- c(0)
 13     -9379,

(as a file to try)

vs. me then obtaining

  constant C_WIENER : integer_vector(0 to 5) :=
  (
    -- c(0)
    -9379,
    -- c(1)

... which is what I would expect for the comments as they comment always the next line and that next line is indented properly, so should the comment - and it actually is.

So as you can see, I'm really not remembering properly what the comment was about.

Given that it looks good actually, maybe let's close the issue here, see about #542 and maybe re-iterate.

[jeremiah-c-leary]

Given that it looks good actually, maybe let's close the issue here, see about #542 and maybe re-iterate.

Sounds good to me.