Skip to content

Conversation

adrientetar
Copy link
Contributor

Thanks to @huonw for some mentoring. 🍰

Copy link
Member

Choose a reason for hiding this comment

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

Hm, if we can avoid an allocation, then why encourage an allocation in the code example? Why not replace the example above with this code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that having more generic code would be good for the tutorial. Do you want me to swap it?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't make sense to me to have two versions of code where you say "this one is better" but you showcase the other one. This suggestion about avoiding allocations should either be dropped or integrated into the other examples in my opinion.

@alexcrichton
Copy link
Member

Nice work, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Isn't the whole point of this section to showcase failure? Why remove the one comment pointing it out that this is the error-handling strategy used in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton Legitimate failure here: the bots want a file with its path path passed as args to test... can I simulate the above with the actual implementation or should I keep the tests xfailed?

Copy link
Member

Choose a reason for hiding this comment

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

I think we've seen what happens when the tests get xfailed, so let's keep them not-xfailed if at all possible. You can take the route of the above code (give a dummy reader) if you want by having mod File hidden at the top.

@adrientetar
Copy link
Contributor Author

r? @alexcrichton

bors added a commit that referenced this pull request Dec 20, 2013
@bors bors closed this Dec 20, 2013
@bors bors merged commit bf5f2f2 into rust-lang:master Dec 20, 2013
@adrientetar adrientetar deleted the patch-new branch January 4, 2014 15:41
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2023
…=Manishearth

Allow safety comment above attributes

Closes rust-lang#8679

changelog: Enhancement: [`undocumented_safety_block`]: Added `accept-comment-above-attributes` configuration.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants