Skip to content

Enable usage of syn from 'panic = abort' code#122

Merged
smklein merged 1 commit intomainfrom
abortable-syn
Jul 15, 2021
Merged

Enable usage of syn from 'panic = abort' code#122
smklein merged 1 commit intomainfrom
abortable-syn

Conversation

@smklein
Copy link
Copy Markdown
Contributor

@smklein smklein commented Jul 15, 2021

Refer to the inline code comment for all the details.

Refer to the inline code comment for all the details.
Copy link
Copy Markdown
Collaborator

@ahl ahl left a comment

Choose a reason for hiding this comment

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

Nice work chasing this down? Is there anything related to the initial bug beyond what you document in the code that's worth capturing in this PR?

Comment thread dropshot/src/router.rs
* Validate that the string is a valid Rust identifier.
*/
fn valid_identifier(var: &str) -> bool {
// TODO: Remove this "Once" callback when the following issue
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FWIW @davepacheco has tried (and failed) to hold the line on /* */ comments in dropshot so... do what you will with that .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I always considered that comment style "valid, but non-idiomatic" - the Rust book only shows the "//" style (see: here) and there was even an (admittedly unsuccessful) attempt to remove block comments: rust-lang/rfcs#1371

(Plus I heard a rustlang maintainer also had the spicy take that they're not idiomatic)

Do you want me to switch before landing? I noticed we're pretty inconsistent across many of Oxide's repos (dunno if we should clarify which repos do / don't have this styling more explicitly).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The rustdoc book also exclusively uses the non-block comment style ("//" or "///")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know.

@smklein
Copy link
Copy Markdown
Contributor Author

smklein commented Jul 15, 2021

Nice work chasing this down? Is there anything related to the initial bug beyond what you document in the code that's worth capturing in this PR?

I don't believe so - it's a bit hairy, and might change if we made other calls to syn/quote from this codebase, but we don't really have a better "initialization point" that wouldn't push this onto dropshot users.

I'm kinda hoping that this gets resolved in upstream rust, fixed in proc_macro2, we can remove the hack, and we'll be done with it.

@smklein smklein merged commit 15acd1a into main Jul 15, 2021
@smklein smklein deleted the abortable-syn branch July 15, 2021 21:54
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.

2 participants