Closed SittingDuc closed 7 months ago
Afternoon @SittingDuc ,
I pushed an update for this to the issue-1065
branch.
I added the option require_blank_line_unless_pragma
to those rules which will enforce a blank line unless there is a pragma on the next line.
Could you give it a try and let me know if there are any changes I need to make?
Thanks,
--Jeremy
Hello, checked out the latest branch and running it on real vhdl..
Component instance (instantiation_019
) is good with the new code - no longer complains about pragma next to the close bracket/ semicolon; and also does not complain if there is a blank line. Good.
component_018
good, process_011
good. I don't have time to test the others, but the new code is behaving as expected/desired for the code I can reach today.
Much thanks,
Morning @SittingDuc ,
I can leave this issue open for a while if you want to spend more time validating the change.
--Jeremy
Hello,
A little further review shows that all the places I have checked (entity declaration end, instantiation start/end, architecture end, process end, generate end, etc) all tolerate a pragma line after themselves, when configured to do so.
However, I then note the rule now behaves like the no_code
option for component_003
and similar where, if a pragma is present after the block, the linter no longer requires a blank line after the pragma. I can live with it, but it does mean ~people around me~ I can create uglier code
-- synthesis translate_off
gen_fred : if true generate
begin
o_wilma <= b_pebbles;
end generate gen_fred;
-- synthesis translate_on
o_betty <= b_bam_bam
with generate_003
set to require blank line
, a blank line is added between the end generate and the pragma. With generate_003
set to require blank line_unless_pragma
, the code as written passes the linter.
What I would ideally love would be a violation with a fix of
end generate gen_fred;
-- synthesis translate_on
o_betty <= b_bam_bam
which I guess makes the semantics "require blank line or pragma" not "unless".
In an effort to be less of a say-er and more of a do-er, I tried reading the commit to see if I could work out how to write the change I want, and I must admit I am better at VHDL than Python.
In summary, the code in PR#1068 does improve the behaviour, and allows me to have my pragmas snug up to the code block they are to jacket; and if technical reasons make the further feature request extension impractical, I can live with that. But if the feature could be extended to then require a blank line after the pragma (or before in the above-code cases, generate_004
and similar), that would be even more powerful.
Thanks again,
Morning @SittingDuc ,
I think there might a different method to handle the desired formatting if we create rules around pragmas. I believe we can divide pragmas into three types: opening
, closing
, and single
.
opening
and closing
refer to a pair of pragmas. For example:
-- synthesis translate_off
-- synthesis translate_on
where -- synthesis translate_off
is the opening
pragma and -- synthesis translate_on
is the closing
pragma.
single
pragmas do not come in pairs. For example:
-- synthesis library <my_lib>
If pragmas are divided into these types then rules can be written against those types:
require blank line before opening
pragma
no blank line after opening
pragma
no blank line before closing
pragma
require blank line after closing
pragma
So now the behavior of the blank lines around opening
and closing
pragmas apply to the pragmas themselves instead of being relative to other productions.
For example:
end component;
-- synthesis translate_off
signal debug_wr_en : std_logic;
signal debug_rd_en ; std_logic;
-- synthesis translate_on
-- synthesis translate_off
signal debug2_wr_en : std_logic;
-- synthesis translate_on
component fifo is
I would think the desired formatting would be:
end component;
-- synthesis translate_off
signal debug_wr_en : std_logic;
signal debug_rd_en ; std_logic;
-- synthesis translate_on
-- synthesis translate_off
signal debug2_wr_en : std_logic;
-- synthesis translate_on
component fifo is
So blank line is added before every opening
pragma and a blank line is added after every closing
pragma.
Blank lines are removed after opening
pragmas and before `closing pragmas.
I believe this would give the compactness of pragmas you are looking for.
What do think?
--Jeremy
Morning @SittingDuc ,
I pushed an update to the issue-1065
branch where I modified the pragma definitions to include close
, open
, and single
styles. I also added four new rules pragma_400
, pragma_401
, pragma_402
, and pragma_403
. These new rules handle blank lines around open
and close
pragmas.
Using a slightly modified version of your code example:
1
2 architecture rtl of fifo is
3
4 begin
5
6 o_ruble <= o_betty;
7 -- synthesis translate_off
8
9 gen_fred : if true generate
10 begin
11 o_wilma <= b_pebbles;
12 end generate gen_fred;
13
14 -- synthesis translate_on
15 o_betty <= b_bam_bam;
16
17 end;
and using this configuration file:
1 rule:
2 generate_004:
3 style: allow_comment
4 generate_003:
5 style: require_blank_line_unless_pragma
The above example code will be fixed to:
1
2 architecture rtl of fifo is
3
4 begin
5
6 o_ruble <= o_betty;
7
8 -- synthesis translate_off
9 gen_fred : if true generate
10 begin
11 o_wilma <= b_pebbles;
12 end generate gen_fred;
13 -- synthesis translate_on
14
15 o_betty <= b_bam_bam;
16
17 end architecture rtl;
Could you check out the new rules and let me know if it gives the formatting you are looking for?
Thanks,
--Jeremy
Hello,
The idea to allocate pragmas based on how they impact the code is a good idea. One drawback is I get python 'keyError' if I don't update my config file:
Migrating existing configurations
"pragma": {
"patterns": [
"^\\s*--\\s+synthesis\\s+\\w+\\s*$",
"^\\s*--\\s+synthesis\\s+\\w+\\s+\\w+\\s*$",
"^\\s*--\\s+pragma\\s+\\w+\\s*$",
"^\\s*--\\s+pragma\\s+\\w+\\s+\\w+\\s*$",
"^\\s*--vsg_[on|off]\\s*$",
]
},
yields
Traceback (most recent call last):
File "/home/duc/.local/bin/vsg", line 8, in <module>
sys.exit(main())
File "/usr/local/lib/python3.10/dist-packages/vsg-3.19.0.dev4-py3.10.egg/vsg/__main__.py", line 110, in main
oConfig = config.New(commandLineArguments)
File "/usr/local/lib/python3.10/dist-packages/vsg-3.19.0.dev4-py3.10.egg/vsg/config.py", line 243, in New
add_pragma_regular_expressions(dConfig)
File "/usr/local/lib/python3.10/dist-packages/vsg-3.19.0.dev4-py3.10.egg/vsg/config.py", line 229, in add_pragma_regular_expressions
for types in list(dStyle['pragma']['patterns'].keys()):
AttributeError: 'list' object has no attribute 'keys'
Might be possible to fix in documentation.
Testing the Changes Once I have a config file updated, the changes all look good. I made an ugly-as vhdl and fed it to the linter and it does what I: cfg_is1065.json is1065_1.vhd.txt
Blank lines above and below the pragmas, and testing it with the default config, all the expected rules report violations (I modified the pragma regexp for this test, purely so I can label each line with the rule name)
Exceptional case
Of course, every beautiful plan is spoiled by an exception, and I found one. While most of the time we are using translate_off
/ translate_on
to jacket a block of code for simulation, in functions in particular, I have found a couple of instances of
function sleep return natural is
begin
-- synthesis translate_off
return 12; -- nap in sim
-- synthesis translate_on
return 2400; -- decent sleep in hardware
end function sleep;
So the semantics in my head returns to "if the thing the pragma is snuggled up to expects a blank line, that blank line request passes to the pragma. If the pragma is not snuggled up to a blank-line-expector, the pragma does not request a blank line either".
For this example, I can probably throw a vsg_off around the function, or just add the blank lines. It might even improve readability by making the simulation block "stand out".
Conclusion The approach for treating pragmas as Opening, Closing, or Indifferent works, and for code that expected blank lines, I can get the behaviour / white-space I want. The one exceptional case feels like more effort than is justified to fix, so I will try to fix it in education; and the 'keyError' migrating from a config file with 3.19.0-pragmas to the new style can probably be fixed in documentation, or even with a more helpful message in config.py (I have found stackdumps help programmers and confuse user)
Thanks once more,
Evening @SittingDuc ,
One drawback is I get python 'keyError' if I don't update my config file:
I added some error checking on configuring pragmas. In your example the following error message will be displayed:
Error in pragma definition:
Refer to "configuring pragmas" section in documentation for details on how to configure pragmas.
Of course, every beautiful plan is spoiled by an exception, and I found one. So the semantics in my head returns to "if the thing the pragma is snuggled up to expects a blank line, that blank line request passes to the pragma. If the pragma is not snuggled up to a blank-line-expector, the pragma does not request a blank line either".
Yeah, implementing that logic would be complicated. The rules are designed to be independent of each other to eliminate dependences. So implementing a rule to have knowledge of another rules configuration would violate the intent of the design.
and the 'keyError' migrating from a config file with 3.19.0-pragmas to the new style can probably be fixed in documentation
The "configuration pragmas" documentation was updated on the branch to show the open
, close
, and single
formatting of the pragmas. It is a little terse, but there is something there.
Hopefully pointing users to that documentation if they followed the old format will be sufficient.
Let me know if you think the error checking I implemented is sufficient.
--Jeremy
Hello,
I understand the philosophy and I can live with the code as it stands.
I tested the error message and it works, and is nicer than the Trackback. All the effort people put in to things that might never be seen
Thanks for all the work, I am happy for this to close / merge. Happy Holidays
Evening @SittingDuc ,
All the effort people put in to things that might never be seen
I used to complain, well...still do, about incomprehensible error messages. Now I have an appreciation for how much work really goes into creating a useful one and why it is so hard to create a useful error message.
I will merge this to master.
--Jeremy
Hello again,
Getting greedy now, this request is low priority / nice-to-have only.
At present, rules like
instantiation_019
force a blank line after an instance,component_018
afterend component
, and so forth. If I put a pragma after a function, component declaration, or instance, it would be visually nicer to have it next to the end declaration, and then the blank line enforced below it.As defined,
component_018
will try to push thevsg_on
line down, making the code less compact and a fraction less readable. Feature request then is some way to make a previously-defined pragma "not count" for the blank line rules, so it can "snug" up to the code it is disabling / modifying, and then the original whitespace rule applies after it (i.e. the blank line remains between the Catapult and the trebuchet components)Not a major concern and I can totally live with it, but if I have noticed, other people might too, now that pragmas are a thing ;)
Thanks once again!