Issue#1492: New rules to handle whitespace in array definitions

[JHertz5]

Resolve #1492.

Opinions may vary on whether the default should be array( or array (. I've gone for the former, but I'm happy to change that if requested.

[jeremiah-c-leary]

I prefer no space before the parenthesis, so the default sounds good to me.

--Jeremy

[maltaisn]

I also think no space is a good default.

I tried setting number_of_spaces: 1 for unbounded_array_definition_100 however and it doesn't always work. Taking the original example:

architecture a of b is

  type my_array is array (natural range <>) of integer; -- OK, fixed to "array ("

  type my_array2 is array (natural range 0 to 7) of std_logic_vector(7 downto 0);  -- Wrong, fixed to "array("

begin

end architecture a;

[JHertz5]

Hi @maltaisn. Thanks for taking a look at the branch. The reason that your code is not being fixed as you'd expect is because (unless I'm mistaken) you've only set the unbounded_array_definition_100 to 1 and haven't also set constrained_array_definition_100 - the definition of my_array is an unbounded_array_definition and the definition of my_array2 is a constrained_array_definition. If you set both rules, you should hopefully see it working.

However I'm very glad that you raised this. In checking your query, I realised that there is an overlap between this PR and PR #1499. The constrained_array_definition_100 rule from this PR overlaps with the index_constraint_100 rule from the other PR. index_constraint_100 is a more useful rule because it covers more cases (constrained array definitions always contain index constraints, but there are also other productions that contain index constraints. I'm going to merge #1499 (which should have been merged already) and remove the conflicting rule from this PR. Thanks for helping me to catch this! I'll make the update shortly and let you know when it's ready for you to try.

[maltaisn]

Ah right I only set the unbounded one. Thanks!

[JHertz5]

@maltaisn I've updated the branch with the newly merged rules from #1488. You should find that in your example snippet, my_array is fixed by unbounded_array_definition_100 and my_array2 is now fixed by index_constraint_100, which is already on the master branch. When you have a chance, could you give the updated branch a try and let me know whether it works for you?

[maltaisn]

It works. So the two following constructs:

1. is array (0 to 1)
2. unbounded_array(0 to 1)

Are now both handled by index_constraint_100. So there's no way to have it like that anymore i.e. 1 space in the first case and 0 the second case?

Not that it matters much. We currently do it like this in our codebase, but we can just use no space if that makes the rules more consistent.

[JHertz5]

@maltaisn Excellent, thanks for checking!

You are correct, index_constraint_100 treats both of those cases in the same way, as they are both index constraints. I'd personally prefer not to break index_constraint_100 down into rules for each possible use of an index constraint (one of which is the constrained array definition that we looked at previously), because there are quite a lot of different productions that use it and it would result in the creation of several new rules, some of which are very niche. It's a trade-off of flexibility vs simplicity. If you'd like, I can have a think and figure out how breaking the rule down might look, but if you're OK with adjusting your style then I (selfishly) suggest that we leave it as is.

[maltaisn]

No it's totally fine this way. Thank you for your work.

[JHertz5]

@maltaisn Excellent, thanks! I'll merge this PR then.