-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios, macos] Change the format for case expressions to a flat structure. #11450
[ios, macos] Change the format for case expressions to a flat structure. #11450
Conversation
NSMutableArray *arguments = [NSMutableArray arrayWithObjects:self.predicate.mgl_jsonExpressionObject, self.trueExpression.mgl_jsonExpressionObject, nil]; | ||
NSMutableArray *arguments = [NSMutableArray arrayWithObjects:self.predicate.mgl_jsonExpressionObject, nil]; | ||
|
||
if (self.trueExpression.expressionType == NSConditionalExpressionType) { |
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.
A simple conditional would look like this:
// If conditional, then trueExpression, else falseExpression
TERNARY(conditional, trueExpression, falseExpression)
A nested conditional would look like this:
// If firstConditional, then firstExpression,
// else if secondConditional, then secondExpression,
// else falseExpression
TERNARY(firstConditional, firstExpression,
TERNARY(secondConditional, secondExpression,
falseExpression))
So it’s the falseExpression
that we need to examine here, not the trueExpression
.
NSExpression *falseExpression; | ||
if (argumentObjects.count > 3) { | ||
if ([argumentObjects[rightBranchIndex] isKindOfClass:[NSArray class]] ) { |
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.
For #11007, we need to implement a new custom NSExpression function that can represent multiple branches. TERNARY()
only supports “if…then”, and “else”, but we need “else if…then” as well.
If NSExpession were to support this feature natively, I would expect it to have initializers similar to +[NSDictionary dictionaryWithObjects:forKeys:]
and +[NSDictionary dictionaryWithObjectsAndKeys:]
, in other words:
+[NSExpression expressionForTrueExpressions:forConditionals:falseExpression:]
+[NSExpression expressionForTrueExpressionsAndConditionals:falseExpression:]
We could implement these methods in this PR to help address #11016 and remove the single-conditional initializer in #11278 (comment).
Once #11015 is implemented, I think the underlying implementation could become clearer, something like MGL_TERNARY(firstConditional, firstExpression, secondConditional, secondExpression, …, falseExpression)
. In the meantime, the existing FUNCTION()
syntax should suffice.
3f39c0c
to
6e3a8fa
Compare
caseArray = [caseArray subarrayWithRange:NSMakeRange(0, caseArray.count - 1)]; | ||
} | ||
|
||
for (int index = 0; index < caseArray.count; index++) { |
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.
-[NSArray count]
returns an NSUInteger
, so to avoid the risk of overflow, index
should be an NSUInteger
.
@@ -258,6 +258,25 @@ - (id)mgl_jsonExpressionObject { | |||
return [self valueForKeyPath:@"mgl_jsonExpressionObject"]; | |||
} | |||
|
|||
- (id)mgl_case { |
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.
This implementation essentially has the developer writing something like this:
[NSExpression expressionWithFormat:@"FUNCTION({firstConditional, firstExpression, secondConditional, secondExpression, …, falseExpression}, 'mgl_case')"]
It’s good that the arguments all sit side by side now instead of requiring nested TERNARY()
expressions. However, placing all the arguments inside a single aggregate expression would be inappropriate, because case
behaves like a control structure rather than a typical mathematical function.
Per #11450 (comment), I think this function should be used more like this:
[NSExpression expressionWithFormat:@"FUNCTION(firstConditional, 'mgl_case:', firstValue, secondConditional, secondValue, …, falseValue)"]
It looks weird, but we need an operand, and it’s natural for the first conditional to be that operand. This design would require a category method of NSPredicate with the following vararg signature:
- (id)mgl_case:(id)firstValue, ...;
As part of #11472, we can implement a control structure–like MGL_SWITCH:
function that’ll have a call site like this:
[NSExpression expressionWithFormat:@"MGL_SWITCH(firstConditional, firstValue, secondConditional, secondValue, ..., falseValue)"]
6e3a8fa
to
40b5edd
Compare
40b5edd
to
a832f82
Compare
Fixes #11007