-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Introduce let / var macros and some further cleanup #1012
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pros:
- Clean
- Familiar if you know Swift
- Consistent between C and C++
Cons:
- Pretty far from the standard names
I say it's worth it. Purists might complain, and we should not be eager to apply this to literally every single line of code that we let into the framework, but it is practical and obvious and safe.
Source/ASTextNode.mm
Outdated
@@ -1308,7 +1308,7 @@ - (NSAttributedString *)_locked_prepareTruncationStringForDrawing:(NSAttributedS | |||
NSDictionary *originalStringAttributes = [originalString attributesAtIndex:originalStringLength-1 effectiveRange:NULL]; | |||
[truncationString enumerateAttributesInRange:NSMakeRange(0, truncationString.length) options:0 usingBlock: | |||
^(NSDictionary *attributes, NSRange range, BOOL *stop) { | |||
NSMutableDictionary *futureTruncationAttributes = [NSMutableDictionary dictionaryWithDictionary:originalStringAttributes]; | |||
NSMutableDictionary *futureTruncationAttributes = [[NSMutableDictionary alloc] initWithDictionary:originalStringAttributes]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're updating this, let's go ahead and use -mutableCopy
throughout. I doubt initWithDictionary:
supports CoW and it's not really meant for this anyway (more about changing class).
@@ -160,7 +160,7 @@ - (ASLayout *)calculateLayoutThatFits:(ASSizeRange)constrainedSize | |||
self.style.descender = stackChildren.back().style.descender; | |||
} | |||
|
|||
auto sublayouts = [[NSMutableArray<ASLayout *> alloc] init]; | |||
const auto sublayouts = [[NSMutableArray<ASLayout *> alloc] init]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be let and same below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberately didn't move the stack layout spec over to the new syntax yet as most of the stack layout specs are c++ from the beginning on and used auto in there.
Source/Private/ASRectMap.mm
Outdated
@@ -62,7 +62,7 @@ - (id)copyWithZone:(NSZone *)zone | |||
|
|||
// { ptr1->rect1 ptr2->rect2 ptr3->rect3 } | |||
NSMutableString *str = [NSMutableString string]; | |||
for (auto it = _map.begin(); it != _map.end(); it++) { | |||
for (var it = _map.begin(); it != _map.end(); it++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use for (let &e : _map) { e.first; e.second }
? It should provide exact same performance with clearer code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep agree - use ref and for each. Changed.
@@ -918,7 +918,7 @@ + (void)sortAndCoalesceItemChanges:(NSMutableArray<_ASHierarchyItemChange *> *)c | |||
ASDisplayNodeAssert(ASHierarchyChangeTypeIsFinal(type), @"Attempt to sort and coalesce item changes of intermediary type %@. Why?", NSStringFromASHierarchyChangeType(type)); | |||
|
|||
// Lookup table [NSIndexPath: AnimationOptions] | |||
NSMutableDictionary *animationOptions = [NSMutableDictionary new]; | |||
NSMutableDictionary *animationOptions = [[NSMutableDictionary alloc] init]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use let animationOptions = [[NSMutableDictionary<NSIndexPath *, NSNumber *> alloc] init];
Source/Base/ASBaseDefines.h
Outdated
# define let auto const | ||
#else | ||
# define var __auto_type | ||
# define let const __auto_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be consistent on ordering: # define let __auto_type const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the const
in front of the auto
. I would say putting the const first is arguably more readable, since it follows English in putting the "adjective" (const) before the "noun" (type).
5310876
to
9f10598
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot's of good changes here, thank you @maicki !
Thanks for review all, it seems we can go forward with it. |
* Introduce let / var and some further cleanup * Address first comments * Update changelog * Move the const before auto
Following up from #968 were we discussed to introduce some standard in terms of defining and initializing objects with type inference. Currently we use
auto
in some places if we are in C++ land, but cannot use it if we not compile for C++. In this places we can use__auto_type
though.This diff introduces a
let
andvar
macro and is defined as:Besides introducing some default, this diff should also give us a way for us to discuss and maybe come to a good solution going forward.