Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Avoid naming struct field like trait method #1294

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Conversation

mgeisler
Copy link
Collaborator

@mgeisler mgeisler commented Oct 3, 2023

The name struct field was confusing because it was named the same as the trait method.

This should clean things up a little. I also compressed the code a bit to make the unimportant structs take up less space.

Fixes #1292.

@mgeisler mgeisler requested a review from djmitche October 3, 2023 10:10
@mgeisler
Copy link
Collaborator Author

mgeisler commented Oct 3, 2023

Hi @njr0, please let me know if the explanation here is clearer?

@njr0
Copy link
Contributor

njr0 commented Oct 3, 2023

Looks great! Thanks!

Copy link
Collaborator

@djmitche djmitche left a comment

Choose a reason for hiding this comment

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

Looks good, just a few ideas for the diagram

: +-----------+-------+ : : | | | '-->| name: "Fido" | :
: : : | | | +---------------+ :
: | capacity | 2 | : : | | | | +-------------+ :
: +-----------+-------+ : : | | | '-->| name: Fido | :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe {name: "Fido"} just to suggest struct-ness and string-ness a bit more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you mean — though the style on the right is consistent with how the Vec fields are shown on the left. I've used this "table style" throughout.

Now, I did try to make Fido be "Fido", but I didn't manage to escape the " for the svgbob plugin. The double quote is used to quote special characters for the plugin.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I have a nicer and more correct solution: I now show the "Fido" string as a separate object. This is consistent with that we do in the diagrams for moved strings.

@njr0
Copy link
Contributor

njr0 commented Oct 3, 2023

I think it would be good to include a mention of vtable...particularly if there were a sentence explaining what it is in the notes.

@njr0
Copy link
Contributor

njr0 commented Oct 4, 2023

Yeah, that's great...the fact that vtable is a short for virtual method table is super-helpful in explaining why you have ::name in it. Obviously, having the field and the method have the same name was confusing, but even after @djmitche said vtable I assumed it stood for variable.

Anyway, the changes altogether seem like a huge upgrade in comprehensibility to me...especially for people who didn't understand it before coming to this section!

The `name` struct field was confusing because it was named the same as
the trait method. The struct fields are now disjoint from the method
names — the fact that there are two separate name spaces is not the
point of these slides.

I also dropped the zero-sized type, this is also not the main focus
here.

I also compressed the code a bit to make the unimportant structs take
up less space.

Fixes #1292.
@mgeisler mgeisler force-pushed the fix-name-field-in-traits branch from a6c9254 to a832013 Compare October 4, 2023 09:47
@mgeisler
Copy link
Collaborator Author

mgeisler commented Oct 4, 2023

I think it would be good to include a mention of vtable...particularly if there were a sentence explaining what it is in the notes.

I added a link to Wikipedia for the vtable.

@mgeisler mgeisler enabled auto-merge (squash) October 4, 2023 09:50
@mgeisler
Copy link
Collaborator Author

mgeisler commented Oct 4, 2023

The new diagram should show up on https://google.github.io/comprehensive-rust/traits/trait-objects.html in a few seconds.

@mgeisler mgeisler merged commit 7395725 into main Oct 4, 2023
@mgeisler mgeisler deleted the fix-name-field-in-traits branch October 4, 2023 09:52
@njr0
Copy link
Contributor

njr0 commented Oct 4, 2023

Yes, that's great. 9 lives 😄.

@djmitche
Copy link
Collaborator

djmitche commented Oct 4, 2023

That's a fantastic result :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix name ambiguity in trait object diagram
3 participants