- 
                Notifications
    
You must be signed in to change notification settings  - Fork 11
 
Multiple commits to ultimately support removal of SafeHandle for global handles #321
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
      
      
    Conversation
  
    
      This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
      Learn more about bidirectional Unicode characters
    
  
  
    
    * Sets the stage for removing use of SafeHandle as it makes the generation of the handle code local to this repo so it's easier to maintain.
* Added scripting to generate the wrappers off-line
* Added specialized target to generate the response file for the generator.
    * The project has a package reference to the LibLLVM package so that it can find the headers to parse.
* Adjusted stylecop.json to get expected behavior with regard to internal interfaces.
    * See: https://github.com/DotNetAnalyzers/
StyleCopAnalyzers/issues/3100
* Fixed `EnsureBuildOutputPaths` target in `Directory.Build.targets` so it actually creates the `NuGet` folder even in IDE builds.
    * Removed unused usings
* Fixed debug record tests to operate without asserts in debug mode * Added wrapper to re-order API parameters for `LLVMOrcJITLookup()` * Renamed `GlobalHandleBaseExtensions.CloneWithAddref()` to `GlobalHandleBaseExtensions.CloneWithManagedAddRef()` to help express and clarify the intent. * Fixed links in docs for clarity * Updated dictionary ignored words list * Clarified/Corrected doc comments * Updated to resolve latest analyzer issues
* Not yet complete but allows merging stashed changes for removal of safehandles.
    * Removing safe handles avoids confusion on addref for managed wrapper and addref for the native ABI handle. These are NOT the same and can't be used interchangeably for `StringPoolEntry` handles as some APIs assume "move" semantics, others have only temp access, while others will perform an addref on their own [It's all VERY confusing and poorly documented - but users of this library should not care about these subtleties. A major point of this lib is to resolve them consistently.
    * Global handles are IDisposable structs
    * While this is sometimes considered controversial it is primarily to leverage built in support for analyzers and use of I Disposable. Additionally it makes the disposal consistent across the handle types.
* Fixed a LOT of issues with reference counting of `SymbolStringPoolEntry`
    * Handling of those was not correct in a LOT of places. This was ignored in a release build of the native libraries, but with a debug build it asserted... a lot! 🤦 This fixes UbiquityDotNET#320
* ContextHandle was renamed to WrappedHandle for both global and aliased handles.
* Code generator was updated to use a single template that generates code for struct handles from the same template. IFF it is a global handle will it be generated with IDisposable support. Ultimately the structs are just a wrapper around a `nint`.
* Added support for C#14 field keyword to make some code simpler and easier to read/maintain.
    * due to bugs in support of `extension` that keywords IS NOT used. It was tried, even in the VS2026 preview, it's just too broken in the IDE and analyzers etc... to be worth the hassle.
        * It isn't necessary for anything other than properties or statics so not all that useful in the end.
* Moved constant enumerations from LibLLVMValueKind to allow debugger to show original value not named symbolic.
* Fixed handling of `LLVMErrorRef` it had ref count and crash issues that were identified in debug builds and testing of conversion to value type for handles.
    * `LLVMErrorRef` itself is a ref type `class` as it is mutable to handle the weird ownership semantics of the error object and message. The wrapper types handle all the idiosyncrasies of this low-level weird behavior.
        * Added special marshaling behavior for the error ref as it isn't like the other handles
         * Added Correct support for TypeId. Technically the only supported form is a string. Other values include an ID but there's nothing available in the LLVM-C API to do anything with them. (It's all very C++ specific). Fortunately they are generally swallowed internally and the only form that is supposed to surface out of the C API is that of a string.
* Added extension property to interop handle `LLVMValueRef` to allow getting the kind of value.
    * This is mostly used  for debugging to find the type of a value while debugging. It isn't generally useful and not exposed from upper layers.
* Added support for getting symbols where the symbol name itself includes the `:` character. Parsing of the string representation is flaky at best and really just a diagnostic/debug tool. (But it was a really helpful one!)
* Added Dispose of the ICodeGenerator to common Kaleidoscope REPL so that it will trigger cleanup (and possible debug asserts for ref count issues)
* Continued application of pattern for naming internal handle in wrapper types.
    * Member is named `Handle` and for types that need to transfer ownership to native there is an `InvalidateAfterMove` to allow for scenarios where move only happens on success.
* Added SrcGeneration library to support source generation and `IndentedTextWriter` extensions.
    * This allows for code only replacement for T4 templates. While T4 is great for some cases it can get unruly and down right impenetrable for more complex cases. Thus, this support library allows for common cases of C# code generators.
        * The handle generator doesn't currently use this as it uses T4 with a fairly simple  template but that may change over time. It is used by `ScopeStack` to format the stack as nested data string for diagnostics.
    
          Test Results291 tests   - 1   176 ✅  - 2   14s ⏱️ +4s Results for commit b33e60b. ± Comparison against base commit ce69e03. This pull request removes 3 and adds 2 tests. Note that renamed tests count towards both. | 
    
  
    Sign up for free
    to join this conversation on GitHub.
    Already have an account?
    Sign in to comment
  
      
  Add this suggestion to a batch that can be applied as a single commit.
  This suggestion is invalid because no changes were made to the code.
  Suggestions cannot be applied while the pull request is closed.
  Suggestions cannot be applied while viewing a subset of changes.
  Only one suggestion per line can be applied in a batch.
  Add this suggestion to a batch that can be applied as a single commit.
  Applying suggestions on deleted lines is not supported.
  You must change the existing code in this line in order to create a valid suggestion.
  Outdated suggestions cannot be applied.
  This suggestion has been applied or marked resolved.
  Suggestions cannot be applied from pending reviews.
  Suggestions cannot be applied on multi-line comments.
  Suggestions cannot be applied while the pull request is queued to merge.
  Suggestion cannot be applied right now. Please check back later.
  
    
  
    
This paves the way for use of arrays of handles without a copy. Though ref count updates are sometimes still required for native code. This has a hard requirement on the native libraries 20.1.8-delta that includes additional diagnostic APIs