VUnit / vunit

VUnit is a unit testing framework for VHDL/SystemVerilog
http://vunit.github.io/
Other
722 stars 260 forks source link

Remove unused code #972

Closed kraigher closed 10 months ago

kraigher commented 11 months ago

This removes a bunch of dead code that I found when developing unused code detection into VHDL LS.

LarsAsplund commented 10 months ago

Nice feature! I like that it can see unused code in several steps. That one line will be unused once you remove another unused line.

I'm thinking such a check should be part of our lint job.

LarsAsplund commented 10 months ago

After the merge I had had a look at what dead code VHDL LS did not find:

check.vhd:77
check.vhd:863
com_deprecated.vhd:912
com_types.vhd:454
common_log_pkg-body.vhd:16
external_integer_vector_pkg.vhd:28
external_integer_vector_pkg.vhd:29
external_integer_vector_pkg.vhd:30
external_integer_vector_pkg.vhd:36
external_integer_vector_pkg.vhd:37
external_integer_vector_pkg.vhd:43
external_string_pkg.vhd:28
external_string_pkg.vhd:29
external_string_pkg.vhd:30
external_string_pkg.vhd:36
external_string_pkg.vhd:37
external_string_pkg.vhd:43
integer_vector_ptr_pkg-body-2002p.vhd:59
integer_vector_ptr_pkg-body-93.vhd:13
location_pkg-body-2008m.vhd:8
run.vhd:389 run.vhd:426 run_tests.vhd:167
string_ptr_pkg-body-2002p.vhd:62
string_ptr_pkg-body-93.vhd:13
sync_pkg-body.vhd:43
tb_avalon_master.vhd:77 tb_avalon_slave.vhd:64
tb_check_relation.vhd:45
tb_check_relation.vhd:59
tb_com_codec.vhd:33 tb_com_codec.vhd:34 tb_com_codec.vhd:35 tb_com_codec.vhd:36 tb_com_codec.vhd:37 tb_com_codec.vhd:44 tb_com_codec.vhd:76 tb_com_codec.vhd:76 tb_com_codec.vhd:78 tb_wishbone_master.vhd:70
tb_wishbone_slave.vhd:63

kraigher commented 10 months ago

Are you basing you conclusion on the contents of this MR? This MR does not remove all dead code within VUnit that VHDL-LS finds. For example it also finds check.vhd:77 that I have not included as part of this MR. Also regarding check.vhd:21 it is the function result which is used a lot so that should not be unused.

Note that the VHDL-LS unused declaration detection only warns for things that are locally unused. Since VHDL does not have a public/private concept it does not assume that things in a package header are unused as it could be intended to be used by a third party. Adding support for globally unused things would require some sideband configuration of what should be considered private and public which I have not implemented. I still think the locally unused stuff is the most important and most likely to find bugs. An unused signal, port, variable of function argument is much more likely to be a bug than an unused function in a package.

LarsAsplund commented 10 months ago

Are you basing you conclusion on the contents of this MR? This MR does not remove all dead code within VUnit that VHDL-LS finds.

I did. I noticed one dead code line and then I created a report from another tool. I didn't check the code in that report but the line I noticed was this

kraigher commented 10 months ago

Yes buf on that line is unused code. But it cannot be removed since allocate has a side effect.

LarsAsplund commented 10 months ago

You're right and the comment in the other tool is that the buf is not used. I also had it setup poorly so it reported on many strange things. Updated the list above.

I still haven't looked at all of them but these lines look correctly reported:

$ git grep -ni null_storage
vunit/vhdl/data_types/src/integer_vector_ptr_pkg-body-2002p.vhd:59:    constant null_storage : storage_t := (integer'low, internal, integer'low);
vunit/vhdl/data_types/src/integer_vector_ptr_pkg-body-93.vhd:13:  constant null_storage : storage_t := (integer'low, internal, integer'low);
vunit/vhdl/data_types/src/string_ptr_pkg-body-2002p.vhd:62:    constant null_storage : storage_t := (integer'low, internal, integer'low);
vunit/vhdl/data_types/src/string_ptr_pkg-body-93.vhd:13:  constant null_storage : storage_t := (integer'low, internal, integer'low);
kraigher commented 10 months ago

Those lines are found by VHDL LS as well.

umarcor commented 6 months ago

See #997.