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.

Sign up for GitHub

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

Jump to bottom

Remove misleading comment and add assertion #trivial #1027

Merged
merged 2 commits into from
Jul 24, 2018
Merged

Remove misleading comment and add assertion #trivial #1027

merged 2 commits into from
Jul 24, 2018

Conversation

wiseoldduck
Copy link
Member

The comment at line 305 strongly implies this is expected to be a dynamic cast.

nguyenhuy
nguyenhuy approved these changes Jul 13, 2018
View reviewed changes
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Adlai-Holler
Adlai-Holler reviewed Jul 13, 2018
View reviewed changes
Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're not seeing crashes, 99% chance that this function is only being called with flattened layouts, which would make sense since unflattened layouts are never "applied."

So I think a better fix is to:

  • Keep the static cast
  • Assert the class is display node
  • Update the function documentation to say that you should only call with flattened layouts.

@nguyenhuy
Copy link
Member

On a second thought, I think @Adlai-Holler is right. These find nodes functions are called with flattened layouts only.

@maicki maicki changed the title #trivial Switch to ASDynamicCast Switch to ASDynamicCast #trivial Jul 14, 2018
nguyenhuy
nguyenhuy requested changes Jul 20, 2018
View reviewed changes
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revoke my approval to reflect my second thought.

@wiseoldduck
Copy link
Member Author

Nooooooooooo!

Yeah no, actually I agree - I forgot I had made this PR. It all makes sense I will adjust.

@wiseoldduck
Copy link
Member Author

wiseoldduck commented Jul 23, 2018 •
edited
Loading

Hmm I believe as of #1006 this method is actually unused, and it is not included in the public header. Maybe we should just remove it
(findNodesInLayoutAtIndexesWithFilteredNodes is used indirectly by the unfiltered findNodesInLayoutAtIndexes)

@ghost
Copy link

ghost commented Jul 23, 2018

🚫 CI failed with log

@maicki
Copy link
Contributor

maicki commented Jul 23, 2018 •
edited
Loading

@wiseoldduck Can you rebase please from latest master and update.

@wiseoldduck
Copy link
Member Author

@maicki Done-and-done

@wiseoldduck wiseoldduck changed the title Switch to ASDynamicCast #trivial Remove misleading comment and add assertion #trivial Jul 23, 2018
nguyenhuy
nguyenhuy approved these changes Jul 24, 2018
View reviewed changes
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@nguyenhuy nguyenhuy merged commit b2eb58e into TextureGroup:master Jul 24, 2018
mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Oct 2, 2018
@wiseoldduck @mikezucc
Remove misleading comment and add assertion #trivial (TextureGroup#1027)
b8c7e18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adlai-Holler Adlai-Holler Adlai-Holler left review comments

@nguyenhuy nguyenhuy nguyenhuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wiseoldduck @nguyenhuy @maicki @Adlai-Holler