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.

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

ref: Add typealias for bytes #2209

Merged
merged 5 commits into from
Sep 24, 2022
Merged

ref: Add typealias for bytes #2209

merged 5 commits into from
Sep 24, 2022

Conversation

kevinrenskers
Copy link
Contributor

This should make it clear what kind of size these variables are. Are they KB, MB, GB? Nope, they are all bytes.

#skip-changelog

@github-actions
Copy link

github-actions bot commented Sep 22, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1217.12 ms 1243.86 ms 26.74 ms
Size 20.51 KiB 331.79 KiB 311.28 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
5025d2e 1248.52 ms 1251.72 ms 3.20 ms
b869536 1250.37 ms 1274.84 ms 24.47 ms
5025d2e 1245.14 ms 1268.58 ms 23.44 ms
9fc2dd0 1246.14 ms 1275.00 ms 28.86 ms

App size

Revision Plain With Sentry Diff
5025d2e 20.51 KiB 331.79 KiB 311.28 KiB
b869536 20.51 KiB 331.79 KiB 311.28 KiB
5025d2e 20.51 KiB 331.79 KiB 311.28 KiB
9fc2dd0 20.50 KiB 331.79 KiB 311.28 KiB

Previous results on branch: fix/bytes-type

Startup times

Revision Plain With Sentry Diff
a5014e6 1247.04 ms 1261.68 ms 14.64 ms
e22634b 1205.16 ms 1233.28 ms 28.12 ms
1e02273 1228.39 ms 1257.07 ms 28.68 ms

App size

Revision Plain With Sentry Diff
a5014e6 20.50 KiB 331.79 KiB 311.29 KiB
e22634b 20.51 KiB 331.79 KiB 311.28 KiB
1e02273 20.51 KiB 331.79 KiB 311.28 KiB

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, please prefix the PR title for refactorings with ref:

Sources/Sentry/include/SentryCrashWrapper.h Outdated Show resolved Hide resolved
@philipphofmann philipphofmann changed the title meta: Add typealias for bytes ref: Add typealias for bytes Sep 22, 2022
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.

LGTM

* master:
  ci: Remove caching for UI tests (#2210)
  ci: Fix broken Benchmark test (#2208)
Copy link
Member

@armcknight armcknight 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 usages of uint64_t in SentryCrashMonitor_System.m that would intermix with this typedef, this only got about half of them. Did you intend to not change those?

Nice change overall. Relatedly, one of the only times I like using a sort of Hungarian notation is to put units like _bytes/_mb/_ms/_minutes/_degrees/_radians on variable/function names. Also relatedly, one of my favorite reasons to use Swift is to be able to use https://github.com/pointfreeco/swift-tagged for even better type safety around this kind of thing.

@kevinrenskers
Copy link
Contributor Author

I must've missed those, I'll take a look tomorrow. And yea I'm a huge fan of everything the Point-Free guys do :)

@brustolin
Copy link
Contributor

TBH Im not a huge fan of this change. The result information should be clear in the documentation, and the function name should be clear.

Having bytes as a value type is confusing, nothing indicates that this is a number indicating the amount of bytes. This could easily be misunderstood as a collection of byte (which btw is a valid type in other languages).

Take - (bytes)freeMemory for example. Am I freeing some memory? Is this function returning the actual bytes released as a collection? What about - (bytes)freeStorage what storage in being released?
I would rather call freeMemorySize, freeStorageSize and 'appMemorySize'

@kevinrenskers
Copy link
Contributor Author

We can do both. If we call it freeStorageSize then question is still "what is the unit", which has come up in multiple comments already.

@brustolin
Copy link
Contributor

IMO a simple

/**
* Returns the amount of bytes...
*/

Above the function in the header should be enough to solve this.

That being said, I will not oppose if you want to merge this. Just sharing my thoughts.

@armcknight
Copy link
Member

We could always add the _bytes suffix to those methods to show that is the unit that is being returned. Alternatively, we could define a struct like

NS_ENUM(NSUInteger, SentryMemoryUnit) {
  SentryMemoryUnitByte,
  SentryMemoryUnitMegabyte,
...
}

typedef struct {
  int quantity;
  SentryMemoryUnit unit;
} SentryMemorySize;

maybe overkill, just looking at alternatives. I'm mostly fine with the proposed typedef. One benefit of having the enum/struct is that doc comments can be attached that can be brought up with a doc popover in Xcode with an ⌥ + click on the names.

@kevinrenskers kevinrenskers merged commit 864c39a into master Sep 24, 2022
@kevinrenskers kevinrenskers deleted the fix/bytes-type branch September 24, 2022 04:29
kevinrenskers added a commit that referenced this pull request Sep 27, 2022
* master:
  release: 7.26.0
  meta: Fix Changelog concurrent transactions (#2229)
  build(deps): bump fastlane from 2.210.0 to 2.210.1 (#2224)
  Revert "feat: profile concurrent transactions (#2105)" (#2225)
  fix: Align core data span operations (#2222)
  test: Remove empty assert msg for AppStateTests (#2221)
  meta: Fix Changelog (#2219)
  ref: Add typealias for bytes (#2209)
  feat: profile concurrent transactions (#2105)
  ci: Readd cache for UI tests (#2215)

# Conflicts:
#	Sentry.xcodeproj/project.pbxproj
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.

4 participants