Skip to content

Conversation

@pepone
Copy link
Member

@pepone pepone commented Oct 29, 2025

Fix #3700

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for enabling static analysis in the C# code generator (slice2cs) and fixes several code quality issues identified by the analyzers.

Key changes:

  • Added --enable-analysis flag to slice2cs compiler to optionally suppress the <auto-generated> tag and enable analyzer warnings
  • Fixed threading issue in ProgressCallback.Report() by directly setting private fields instead of using properties
  • Improved code clarity with better variable naming and added parentheses for operator precedence
  • Added missing period in documentation comment

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
slice/IceGrid/Descriptor.ice Added missing period to documentation comment
csharp/test/Ice/executor/AllTests.cs Renamed parameter from sentSynchronously to value for clarity
csharp/test/Ice/ami/AllTests.cs Fixed concurrency issue by directly setting backing fields in Report() method
csharp/test/Directory.Build.props Added MSBuild configuration to pass --enable-analysis flag to slice compiler
csharp/src/Directory.Build.props Added MSBuild configuration to pass --enable-analysis flag to slice compiler
cpp/src/slice2cs/Main.cpp Added command-line option --enable-analysis and passes it to Gen constructor
cpp/src/slice2cs/Gen.h Updated Gen constructor signature to accept enableAnalysis parameter
cpp/src/slice2cs/Gen.cpp Implemented analysis mode logic and applied analyzer-suggested improvements
cpp/src/slice2cs/CsUtil.cpp Added parentheses for clarity in arithmetic expressions
.github/workflows/ci.yml Enabled analysis in Windows debug build configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +60 to +61
_sentSynchronously = value;
_sent = true;
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Report() method directly sets the backing fields without acquiring the lock on _mutex, while the corresponding properties use locking. This creates a race condition where one thread could read via the properties while another writes via Report(). The direct field assignments should be wrapped in lock (_mutex) to maintain thread safety.

Suggested change
_sentSynchronously = value;
_sent = true;
lock (_mutex)
{
_sentSynchronously = value;
_sent = true;
}

Copilot uses AI. Check for mistakes.
_out << nl << "#pragma warning disable SA1611 // The documentation for parameter x is missing";
if (_enableAnalysis)
{
// Disable some warnings when auto-generated is removed from the header. See printGeneratedHeader above.
Copy link

Copilot AI Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment references 'when auto-generated is removed from the header', but the logic shows this code runs when _enableAnalysis is true, which is when the auto-generated tag is NOT included (removed). Consider rephrasing to 'when auto-generated tag is not included in the header' for clarity.

Suggested change
// Disable some warnings when auto-generated is removed from the header. See printGeneratedHeader above.
// Disable some warnings when the auto-generated tag is not included in the header. See printGeneratedHeader above.

Copilot uses AI. Check for mistakes.
"--depend-xml Generate dependencies in XML format.\n"
"--depend-file FILE Write dependencies to FILE instead of standard output.\n"
"--enable-analysis Enable static analysis for generated code by not including\n"
" <auto-generated> tag which suppresses analyzer warnings.\n"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are potentially multiple tags involved (one per file).

Suggested change
" <auto-generated> tag which suppresses analyzer warnings.\n"
" <auto-generated> tags which suppresses analyzer warnings.\n"

Comment on lines +60 to +61
_sentSynchronously = value;
_sent = true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with this specific test, but it does seem a little suspicious not using the mutex here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

slice2cs --enable-analysis

2 participants