Skip to content

WI #2784 Avoid use of non readonly static fields #2786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 22, 2025

Conversation

fm-117
Copy link
Contributor

@fm-117 fm-117 commented Jul 16, 2025

All cases have been corrected except:

  • FunctionCallResult.callSiteCounter in StorageArea.cs, line 391
  • SecureObjectPool.s_poolUserIdCounter in ImmutableList.cs, line 5727

Both are used to generate unique ids and are legitimate as their only usages are through Intelrocked.Increment(ref int) method which prevent concurrency issues when accessing from multiple threads.

About performance test: the result file was outdated, apparently since 995dda4. Updating it is not strictly part of this issue.

@fm-117 fm-117 requested a review from efr15 July 16, 2025 14:07
@fm-117 fm-117 self-assigned this Jul 16, 2025
@fm-117 fm-117 linked an issue Jul 16, 2025 that may be closed by this pull request
@efr15
Copy link
Contributor

efr15 commented Jul 18, 2025

Could you please explain why you remove the static usage of field AntlrPerformanceProfiler in CodeElementsParserStep?

Copy link
Contributor

@efr15 efr15 left a comment

Choose a reason for hiding this comment

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

There are still some static fields which are not read-only in projects CodeGen (example: Skeletons.NodeActionsProviderMap) and TypeCobol.Transform.
Maybe we can change them as well.

@fm-117 fm-117 requested a review from efr15 July 21, 2025 07:31
@fm-117
Copy link
Contributor Author

fm-117 commented Jul 21, 2025

Could you please explain why you remove the static usage of field AntlrPerformanceProfiler in CodeElementsParserStep?

Initially the ANTLR stats object was stored on a single static reference which is incorrect because the stats can be computed each time the ANTLR parser is called. Now it is attached to the PerfsStatsForParserInvocation object which is more in-line with its role. It allows to compare stats between first time parsing and last time parsing for example (even though we don't use this for now).

As a drawback the PerfsStatsForParserInvocation object is slightly bigger, holding one more reference. Also, the combination of ANTLR stats is not implemented so the stats won't be available on TotalParsingTime.

@fm-117 fm-117 merged commit fe42e58 into develop Jul 22, 2025
8 checks passed
@fm-117 fm-117 deleted the 2784-avoid-use-of-non-readonly-static-fields branch July 22, 2025 06:57
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.

2 participants