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

[sconwayaus]

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]

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]

Created #2193 to investigate.

[hzeller]

Looks good, but I'd be careful changing the default behavior in one go.

I've updated the CI to use 2019, so hopefully it will pass now (and if not in this PR, we can ignore a Windows test failure)

[hzeller]

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).

[hzeller]

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.

[sconwayaus]

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 <#2190 (comment)> : > @@ -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_allow_parameter_) { + 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 <#2190 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVJSI3ZSKEU6TWSVGMGOPTZG4YJFAVCNFSM6AAAAABI4S2OM2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJRGA3TGMZQGA> . You are receiving this because you authored the thread.Message ID: ***@***.***>