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.

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

codegen: Add an option to skip comment generation. #444

Merged
merged 2 commits into from
Jan 26, 2017

Conversation

emilio
Copy link
Contributor

@emilio emilio commented Jan 26, 2017

This is mostly a work around #426,
until we implement the proper fix.

r? @fitzgen

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

r=me with nitpicks addressed

Thanks!

@@ -1007,8 +1011,10 @@ impl CodeGenerator for CompInfo {
};

let mut attrs = vec![];
if let Some(comment) = field.comment() {
attrs.push(attributes::doc(comment));
if ctx.options().generate_comments {
Copy link
Member

Choose a reason for hiding this comment

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

This is a whole lot of copy paste... What if we made Item::comment and Field::comment take the context and check themselves? The earlier we push this check, we could even potentially avoid keeping the comments around in memory.

I would be fine with landing this code now and investigating this later, but if you do want to land the code now, please file an issue and maybe mark it as a good first bug to attract new contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, but I thought that the alternative would be less readable? Anyway, will file.

@@ -1,6 +1,6 @@
[root]
name = "bindgen"
version = "0.20.2"
version = "0.20.3"
Copy link
Member

Choose a reason for hiding this comment

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

This should ideally be part of the previous commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, moved.

/// Whether to whitelist types recursively or not. Defaults to true.
///
/// See https://github.com/servo/rust-bindgen/issues/429 for different use
/// cases for this.
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 should elaborate the use cases here: one shouldn't have to jump from the rustdocs to a random github issue to understand why a method exists and to figure out if they should use it or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough :)

@emilio
Copy link
Contributor Author

emilio commented Jan 26, 2017

@bors-servo r=fitzgen

@bors-servo
Copy link

📌 Commit 78e425f has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit 78e425f with merge 41135ca...

bors-servo pushed a commit that referenced this pull request Jan 26, 2017
codegen: Add an option to skip comment generation.

This is mostly a work around #426,
until we implement the proper fix.

r? @fitzgen
@bors-servo
Copy link

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit 78e425f into rust-lang:master Jan 26, 2017
@emilio emilio deleted the no-comments branch January 26, 2017 20:06
@ghost
Copy link

ghost commented Jan 26, 2017

This is much appreciated. Thanks!

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.

4 participants