[proper-parameter-declaration] parameter vs localparam in packages

sconwayaus commented 3 weeks ago

Resolves #2160

This PR allows the user to configure the proper-parameter-declaration rule to allow either or both parameter and localparam in packages.

The default behaviour has changed to be more inline with the LRM, where the LRM encourages users to use localparam in packages as they cannot be overridden, so the rule defaults to only allow localparam's within packages.

sconwayaus commented 3 weeks ago

The windows CI build seems to have broken with the latest runner release (windows2022 runner was updated 3rd June 2024).

The build seems to be failing due to a number of missing standard libraries. Example below:

ERROR: D:/a/verible/verible/common/util/BUILD:101:11: Compiling common/util/init_command_line.cc [for tool] failed: (Exit 1): clang-cl.exe failed: error executing command (from target //common/util:init-command-line) C:\Program Files\LLVM\bin\clang-cl.exe /nologo /DCOMPILER_MSVC /DNOMINMAX /D_WIN32_WINNT=0x0601 /D_CRT_SECURE_NO_DEPRECATE /D_CRT_SECURE_NO_WARNINGS /bigobj /Zm500 /EHsc /wd4351 /wd4291 /wd4250 /wd4996 ... (remaining 21 arguments skipped)
In file included from common/util/init_command_line.cc:15:
.\common/util/init_command_line.h(18,10): fatal error: 'string' file not found
#include <string>
         ^~~~~~~~
1 error generated.

I've tried the following:

Manually trigger the CI on an unmodified branch. Fails in a similar way.
Changing the runner to windows 2019 seems to build.
There is some conversation happening here https://github.com/actions/runner-images/issues about issues with clang-cl but nothing exactly like what I'm observing.

@corco, maybe you have some thoughts?

hzeller commented 3 weeks ago

Created #2193 to investigate.

hzeller commented 2 weeks ago

Thanks!

Can you now add a follow-up Pull Request so that we can change the default to warn about using parameter in packages (I really like the feature, just important to change code from changing default configuration).

sconwayaus commented 2 weeks ago

I like it. I'll take a look.

On Wed, 12 June 2024, 3:34 am Henner Zeller, @.***> wrote:

@.**** commented on this pull request.

In verilog/analysis/checkers/proper_parameter_declaration_rule.cc https://github.com/chipsalliance/verible/pull/2190#discussion_r1635255985 :

@@ -79,11 +122,24 @@ void ProperParameterDeclarationRule::HandleSymbol( } else if (ContextIsInsideModule(context) && !ContextIsInsideFormalParameterList(context)) { violations_.insert(LintViolation(symbol, kParameterMessage, context));

} else if (ContextIsInsidePackage(context)) {

if (!package_allowparameter) {

violations_.insert(LintViolation(symbol, kParameterMessage, context));

}

We have a opportunity here to provide an autofix to change parameter to localparam. That way, in the language server it is just one click away to get fixed.

Might be a good follow-up change.

— Reply to this email directly, view it on GitHub https://github.com/chipsalliance/verible/pull/2190#pullrequestreview-2111073300, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVJSI3ZSKEU6TWSVGMGOPTZG4YJFAVCNFSM6AAAAABI4S2OM2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJRGA3TGMZQGA . You are receiving this because you authored the thread.Message ID: @.***>

chipsalliance / verible

[proper-parameter-declaration] parameter vs localparam in packages #2190

@.**** commented on this pull request.