Skip to content

Conversation

@phunkeler
Copy link
Contributor

@phunkeler phunkeler commented Jan 6, 2025

This is meant to address #3690

As you can see, this doesn't really clean things up -- mainly to preserve regex source generation for net8.0 (Option 1).

If we're fine using dynamic regex compilation at runtime for net8.0 (Option 2) things get much simpler but it may come with performance changes.

OriginHelper.cs (As-is)
#if NET8_0_OR_GREATER 
     [GeneratedRegex(ValidOriginPattern, RegexOptions.Compiled)] 
     private static partial Regex ValidOriginRegEx(); 
     private static readonly Regex ValidOrigin = ValidOriginRegEx(); 
 #else 
     private static readonly Regex ValidOrigin = new (ValidOriginPattern, RegexOptions.Compiled); 
 #endif 
OriginHelper.cs (Option 1)
#if NET9_0_OR_GREATER
    [GeneratedRegex(ValidOriginPattern)]
    private static partial Regex ValidOrigin { get; }
#elif NET8_0
    [GeneratedRegex(ValidOriginPattern)]
    private static partial Regex ValidOriginRegex();
    private static readonly Regex ValidOrigin = ValidOriginRegex();
#else
    private static readonly Regex ValidOrigin = new(ValidOriginPattern, RegexOptions.Compiled);
#endif
OriginHelper.cs (Option 2)
#if NET9_0_OR_GREATER
    [GeneratedRegex(ValidOriginPattern)]
    private static partial Regex ValidOrigin { get; }
#else
    private static readonly Regex ValidOrigin = new(ValidOriginPattern, RegexOptions.Compiled);
#endif

Thoughts?

#skip-changelog

@jamescrosswell
Copy link
Collaborator

Thanks heaps for the PR @phunkeler !

As you can see, this doesn't really clean things up

It's a pity. I'd thought what enabled [GeneratedRegex] properties was the new support for partial properties in C#13... Presumably GeneratedRegex has also changed so that this can be applied to properties and not methods - which I'm guessing needs the net9.0 runtime.

Thoughts?

I'd say stick with Option 1 for the time being, so that people targeting net8.0 also get the performance improvements of generated regex. It makes the code a bit more verbose in the short term but when net8.0 reaches EOL we can simplify it and so we'll eventually end up with Option 2.

@codecov
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 96.15385% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.47%. Comparing base (495e742) to head (7a230a3).
Report is 431 commits behind head on main.

Files with missing lines Patch % Lines
src/Sentry/SentryMonitorOptions.cs 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3870      +/-   ##
==========================================
+ Coverage   75.73%   76.47%   +0.74%     
==========================================
  Files         357      389      +32     
  Lines       13466    14226     +760     
  Branches     2671     2860     +189     
==========================================
+ Hits        10198    10880     +682     
- Misses       2593     2652      +59     
- Partials      675      694      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jamescrosswell
Copy link
Collaborator

btw: The builds on Ubuntu are failing periodically because of this - I'm working on a fix in that branch, which you should get for free once that's all green and merged into main.

@phunkeler phunkeler marked this pull request as ready for review January 6, 2025 14:12
Copy link
Collaborator

@jamescrosswell jamescrosswell left a comment

Choose a reason for hiding this comment

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

Thanks heaps for the PR @phunkeler !

@jamescrosswell jamescrosswell merged commit c2c737b into getsentry:main Jan 9, 2025
23 checks passed
@jamescrosswell jamescrosswell linked an issue Jan 16, 2025 that may be closed by this pull request
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.

Add [GeneratedRegex] properties

2 participants