Introduce let / var macros and some further cleanup #1012
Conversation
There was a problem hiding this comment.
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.
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.
Should this be let and same below?
There was a problem hiding this comment.
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.
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.
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.
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.
Let's be consistent on ordering: # define let __auto_type const
There was a problem hiding this comment.
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).
maicki
force-pushed
the
MSLetVarAndFurtherCleanup
branch
from
5310876
to
9f10598
Compare
|
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
autoin 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_typethough.This diff introduces a
letandvarmacro 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.