Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ref: functions to convert to/from enum cases #2108

Merged
merged 10 commits into from
Sep 9, 2022
Merged

ref: functions to convert to/from enum cases #2108

merged 10 commits into from
Sep 9, 2022

Conversation

armcknight
Copy link
Member

Closes #2088

  • Various dead code removal, method deduplication, test fixes.
  • Convert ObjC methods to C functions for performance.

Each enum handled in a separate commit, recommend viewing the PR one commit at a time.

#skip-changelog

@github-actions
Copy link

github-actions bot commented Aug 30, 2022 •
edited
Loading

Performance metrics 🚀

Plain With Sentry Diff
Startup time (ms) 1254.73 1281.04 26.31
Size (bytes) 21157 333309 312152

@armcknight armcknight force-pushed the armcknight/enum-case-names branch 5 times, most recently from 781c867 to e4cf66b Compare August 30, 2022 23:36
@armcknight
Copy link
Member Author

I'm stumped as to why there are duplicate symbols for the exported constants in SentryDiscardReasonMapper.h. It has the same implementation pattern as e.g. SentryDataCategoryMapper.h. Those headers and the corresponding headers defining the enums are in the same Headers Build Phase for the framework target. The test apps iOS-Swift et al build fine.

@philipphofmann any ideas? I can start pulling apart the integration tests to test them locally but not sure if that'll actually help me.

philipphofmann
philipphofmann reviewed Sep 6, 2022
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks, I like the refactoring a lot. Can we merge this soon? Otherwise, we could get hard to resolve conflicts easily.

Sources/Sentry/Public/SentryDefines.h Show resolved Hide resolved
@armcknight armcknight marked this pull request as ready for review September 6, 2022 18:16
@armcknight
Copy link
Member Author

Ah i forgot to mark this as ready for review before I left. It's ready now @philipphofmann 😄

@armcknight armcknight mentioned this pull request Sep 7, 2022
Merged
brustolin
brustolin reviewed Sep 7, 2022
View reviewed changes
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

This is nice. Im a little worried about all the generic names used for global symbols.

armcknight and others added 8 commits September 8, 2022 09:56
@armcknight
ref: add function to return level name from enum constant
58475e8 
- remove duplicate tests
- add tests for going from name->level
- use C functions instead of ObjC methods
@armcknight
ref: add function to return sampler decision name from enum constant
8cc307b
@armcknight
ref: add function to return discard reason name from enum constant
d670e02
@armcknight
ref: add function to return span status name from enum constant
542e565
@armcknight
ref: add function to return data category name from enum constant
b07e8ad 
- deduplicated method that parses data categories for rate limits with one mod, which is inlined at the call site
- add tests and add degenerate cases for converting an unexpected string/numeric value to enum case
@getsentry-bot @armcknight
Format code
e9e2631
@armcknight
fix generic names to include more complete name of enum being mapped
544c22a
@armcknight
more pr feedback
5650f77
@armcknight armcknight force-pushed the armcknight/enum-case-names branch from 883003b to 5650f77 Compare September 8, 2022 17:56
brustolin
brustolin approved these changes Sep 9, 2022
View reviewed changes
Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

philipphofmann
philipphofmann approved these changes Sep 9, 2022
View reviewed changes
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for this lovely refactoring, @armcknight.

Sources/Sentry/SentryDiscardReasonMapper.m

NS_ASSUME_NONNULL_BEGIN
Copy link
Member

Choose a reason for hiding this comment

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

l: Why don't we keep NS_ASSUME_NONNULL_BEGIN?

Copy link
Member Author

@armcknight armcknight Sep 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It only really makes sense for header files, or maybe prototypes, interface extensions or block type declarations in an implementation file. Even the _Nonnull in this file I copied accidentally from the prototype in the header and could be removed.

Sources/Sentry/SentryRateLimitParser.m

// Unknown categories must be ignored
if (category != kSentryDataCategoryUnknown) {
// Unknown categories must be ignored. UserFeedback is not listed for rate limits, see
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for keeping this important comment.

@armcknight armcknight merged commit 10a83a9 into master Sep 9, 2022
@armcknight armcknight deleted the armcknight/enum-case-names branch September 9, 2022 09:56
kevinrenskers added a commit that referenced this pull request Sep 15, 2022
@kevinrenskers
Merge branch 'master' into fix/1072-fix-double-quote-warnings
e05b919 
* master: (73 commits)
  ref: Fix typo in measurement (#2154)
  Fix typos in changelog (#2153)
  docs: Add a note on flaky tests to Contributing (#2161)
  docs: fix PR reference in CHANGELOG (#2157)
  test: generate graphs for benchmarks (#2158)
  release: 7.25.1
  fix: Prewarmed app start detection (#2151)
  ci: temporarily disable scheduled CI runs uploading profiles/symbols (#2152)
  Run tests on iOS 16 using Xcode 14.0 (#2147)
  ref: Fix Xcode 14 compile issues (#2145)
  Provide fallbacks for assertions in production (#2141)
  Fix compile errors in Xcode 14 (#2146)
  release: 7.25.0
  fix: setting SDK name through options[sdk][name] shouldn't clear version (#2139)
  fix: SentryTracer instances should be called "tracer", not "transaction" (#2137)
  ref: functions to convert to/from enum cases (#2108)
  fix: Crash with screenshot is reported twice (#2134)
  meta: Clarify PR rules in Contributing (#2133)
  meta: Fix Changelog (#2136)
  feat: Add support for dynamic library (#1726)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brustolin brustolin brustolin approved these changes

@philipphofmann philipphofmann philipphofmann approved these changes

@indragiek indragiek Awaiting requested review from indragiek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add string mapper methods for enums
4 participants
@armcknight @philipphofmann @brustolin @getsentry-bot