Skip to content

Conversation

filipnavara
Copy link
Member

@filipnavara filipnavara commented Feb 7, 2025

Addresses #99109 (comment):

BitVector does not seem to be used anywhere else. What is the advantage of using the BITVECTOR wrapper? Can we delete bitvector.h/bitvector.cpp and turn the handful of methods into internal helpers used by x86 GC info decoding only?

I would move it together (and maybe renamed it too). It is not a general purpose bitvector. Also, delete the non-BitVector path.

Fixes #112251

@ghost ghost added the area-VM-coreclr label Feb 7, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 7, 2025
@filipnavara
Copy link
Member Author

@jkotas PTAL
@MichalStrehovsky What's the recommended way to run the affected test? It doesn't fail debug runs on my machine but I didn't check if there's any exclusion.

@MichalStrehovsky
Copy link
Member

What's the recommended way to run the affected test? It doesn't fail debug runs on my machine but I didn't check if there's any exclusion.

Did you add priority 1 to the build.cmd command line building the tests? The default is pri0 tests only and pri1 won't build.

}
}

friend BOOL isZero(const ptrArgTP& arg)
Copy link
Member

Choose a reason for hiding this comment

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

What are these friends for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I took it from the original bitvector. These methods are not used as member methods. I can change it but I wanted to limit the number of changes.

Copy link
Member

@jkotas jkotas Feb 7, 2025

Choose a reason for hiding this comment

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

I meant what does friend modifier on a method implementation do in C++? I am surprised that it compiles, but I cannot think of what it can do.

I would not mind deleting unnecessary cruft. I agree that it is best to avoid any logic changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is basically equivalent to declaring friend BOOL isZero(const ptrArgTP& arg); here and then placing the method itself outside of the struct/class.

@filipnavara filipnavara marked this pull request as ready for review February 7, 2025 13:13
@jkotas
Copy link
Member

jkotas commented Feb 7, 2025

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Feb 7, 2025

Opened #112289 on the failure

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas jkotas merged commit 61b9cb8 into dotnet:main Feb 7, 2025
114 of 116 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Feb 10, 2025
* main: (41 commits)
  Automated bump of chrome version (dotnet#112309)
  Add `GetDeclaringType` to `PropertyDefinition` and `EventDefinition`. (dotnet#111646)
  Update the System.ComponentModel.Annotations solution to build in VS (dotnet#112313)
  JIT: initial support for stack allocating arrays of GC type (dotnet#112250)
  [main] Update dependencies from dotnet/roslyn (dotnet#112260)
  Update Xcode casing (dotnet#112307)
  update the location of assert for REG_ZR check (dotnet#112294)
  Enable `SA1206`: Keyword ordering (dotnet#112303)
  Address feedback on dense FrozenDictionary optimization (dotnet#112298)
  Start regular pri-1 tests runs with native AOT (dotnet#111391)
  Observe exceptions from _connectionCloseTcs (dotnet#112190)
  Test failure - SendAsync_RequestVersion20_ResponseVersion20 (dotnet#112232)
  Fix init race in mono_class_try_get_[shortname]_class. (dotnet#112282)
  Remove repeated call to DllMain (dotnet#112285)
  Replace bitvector.h/cpp with ptrArgTP type in gc_unwind_x86.h/inl (dotnet#112268)
  JIT: Limit 3-opt to 1000 swaps per run (dotnet#112259)
  [main] Update dependencies from dotnet/icu, dotnet/runtime-assets (dotnet#112120)
  Update dependencies from https://github.com/dotnet/emsdk build 20250205.3 (dotnet#112223)
  Fix EventPipe on Android CoreClr. (dotnet#112270)
  Fix exception handling in the prestub worker (dotnet#111937)
  ...
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2025
@filipnavara filipnavara deleted the ptrArgTP branch April 2, 2025 20:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-VM-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Assertion argCnt < MAX_PTRARG_OFS failed

3 participants