-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
refactor(ivy): misc #23351
refactor(ivy): misc #23351
Conversation
You can preview 770a777 at https://pr23351-770a777.ngbuilds.io/. |
/** Whether or not this node is a component */ | ||
Component = 0b001, | ||
/** The number of directives on this node is encoded on the least significant bits */ | ||
NbOfDirectivesMask = 0b00000000000000000000111111111111, |
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.
How about DirSizeMask
? Find Nb
kind of hard to read.
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.
DirSize
is confusing IMO ? It could mean that directive have size.
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 keep number
, can we write it out then as number
? Nb
took me a second to understand since it's not the normal abbreviation.
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.
How about just DirectiveCountMask
?
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.
Ah, count. That is clearer than size.
/** Mask to get the number of directives on this node */ | ||
SIZE_MASK = 0b00000000000000000001111111111110 | ||
/** The index of the first directive on this node is encoded on the most significant bits */ | ||
IndexOfFirstDirectiveShift = 13, |
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.
How about DirIndexShift
? This is kind of long and hard to read.
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.
Same kind or remark.
More explicit does not hurt
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'm just concerned that the trade-off is readability, but I don't feel strongly
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.
how about DirectiveStartingIndexShift
ngDevMode && assertNotEqual( | ||
previousOrParentNode.tNode !.flags & TNodeFlags.NbOfDirectivesMask, | ||
TNodeFlags.NbOfDirectivesMask, 'Reached the max number of directives'); | ||
previousOrParentNode.tNode !.flags++; |
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.
Are you saving the first index? Looks like it's deleted here, which is probably why the tests are failing.
We should only be adding to the size this way if the first directive index has already been saved by a previous directive.
@kara PTAL |
You can preview 5247915 at https://pr23351-5247915.ngbuilds.io/. |
You can preview d824e05 at https://pr23351-d824e05.ngbuilds.io/. |
You can preview e8b265a at https://pr23351-e8b265a.ngbuilds.io/. |
You can preview f39ed82 at https://pr23351-f39ed82.ngbuilds.io/. |
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.
LGTM
packages/core/src/render3/di.ts
Outdated
// Get the definition for the directive at this index and, if it is injectable (diPublic), | ||
// and matches the given token, return the directive instance. | ||
const directiveDef = defs[i] as DirectiveDef<any>; | ||
if (directiveDef.diPublic && directiveDef.type == token) { | ||
if (directiveDef.type == token && directiveDef.diPublic) { |
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.
Shouldn't this use triple-equals comparison? It is now different from getIdxOfMatchingDirective
in render3/query.ts
packages/core/src/render3/query.ts
Outdated
@@ -196,10 +196,12 @@ function getIdxOfMatchingSelector(tNode: TNode, selector: string): number|null { | |||
function geIdxOfMatchingDirective(node: LNode, type: Type<any>): number|null { |
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 should probably be named getIdxOfMatchingDirective
You can preview 6f13bbc at https://pr23351-6f13bbc.ngbuilds.io/. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.