Implements Debug trait for types which do not support derive Debug #899
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For types that do not support derive Debug be implemented automatically by rust, we know can generate implementations of the Debug trait. This code generation is hidden behind the '--force-derive-debug' command-line flag.
bkchr
force-pushed
the
manual_debug_impl
branch
from
7fedb6f
to
b541f4c
Compare
|
Huh. Linking errors in |
|
@bkchr thanks for this PR! I'm taking a look now |
fitzgen
reviewed
Aug 10, 2017
| kind: CompKind, | ||
| ) -> Vec<ast::ImplItem> { | ||
| let struct_name = item.canonical_name(ctx); | ||
| let mut format_string = format!("{} {{{{ ", struct_name); |
There was a problem hiding this comment.
I could remove some mustaches by doing format_string.push_str("{{"). Or just leave it as it is?
src/codegen/derive_debug.rs
Outdated
| fn gen_bitfield_unit_debug_impl( | ||
| ctx: &BindgenContext, | ||
| data: &BitfieldUnit, | ||
| ) -> Option<(String, Vec<TokenTree>)> { |
There was a problem hiding this comment.
All of these methods have pretty much the same signature:
fn (&BindgenContext, &T) -> Option<(String, Vec<TokenTree>)>;
// or sometimes
fn (&BindgenContext, &T, &U) -> Option<(String, Vec<TokenTree)>;Let's formalize this pattern with a trait:
/// A trait for the things which we can codegen tokens that contribute towards a
/// generated `impl Debug`.
pub trait ImplDebug {
/// Any extra parameter required by this a particular `ImplDebug` implementation. This
/// is the `U` from the above snippet, or `()` if its not needed.
type Extra;
/// Generate a format string snippet to be included in the larger `impl Debug`
/// format string, and the code to get the format string's interpolation values.
fn impl_debug(&self, ctx: &BindgenContext, extra: &Self::Extra) -> Option<(String, Vec<TokenTree>)>;
}And then each of these functions should be impl ImplDebug for BitfieldUnit {...} etc.
There was a problem hiding this comment.
Okay, will do it :) I also thought about creating a Trait, but that seemed to be to much overengineering for me.
src/codegen/derive_debug.rs
Outdated
|
|
||
| TypeKind::Array(_, len) => { | ||
| // Generics are not required to implement Debug | ||
| if ctx.lookup_item_id_has_type_param_in_array(&item.id()) { |
There was a problem hiding this comment.
This should be:
if item.has_type_param_in_array(ctx) {
...
}You may have to add use ir::analysis::HasTypeParamInArray; to the top of the file.
src/options.rs
Outdated
| @@ -55,6 +56,9 @@ pub fn builder_from_flags<I> | |||
| Arg::with_name("no-derive-debug") | |||
| .long("no-derive-debug") | |||
| .help("Avoid deriving Debug on any type."), | |||
| Arg::with_name("force-derive-debug") | |||
| .long("force-derive-debug") | |||
There was a problem hiding this comment.
Let's call this flag impl-debug.
| enumerate ( ) . map ( | ||
| | ( i , v ) | format ! ( | ||
| "{}{:?}" , if i > 0 { ", " } else { "" } , v ) ) . collect :: < | ||
| String > ( )) |
There was a problem hiding this comment.
Filed #900 to investigate running rustfmt on the code we emit :)
| @@ -0,0 +1,7 @@ | |||
| // bindgen-flags: --opaque-type "Opaque" --force-derive-debug | |||
There was a problem hiding this comment.
Don't think the --opaque-type is needed here.
There was a problem hiding this comment.
Ohh that is clearly a copy paste error :D
| // bindgen-flags: --force-derive-debug | ||
|
|
||
| template<typename T, int N> | ||
| class Opaque { |
There was a problem hiding this comment.
It's worth adding a comment that this is opaque because of the non-type template parameter, which is easy to miss.
There was a problem hiding this comment.
Ahh nice to know, I overseved the behavior with all the other tests while developing. But I could not find the right position in the code where this case is handled.
bkchr
force-pushed
the
manual_debug_impl
branch
from
a0634d6
to
f7345fa
Compare
|
I addressed your comments :) |
fitzgen
approved these changes
Aug 10, 2017
|
@bors-servo r+ |
|
📌 Commit f7345fa has been approved by |
bors-servo
pushed a commit
that referenced
this pull request
Aug 10, 2017
Implements Debug trait for types which do not support derive Debug For types that do not support derive Debug be implemented automatically by rust, we know can generate implementations of the Debug trait. This code generation is hidden behind the '--force-derive-debug' command-line flag. Should solve: #875 Sorry for the extra noise in lib.rs, codegen/mod.rs etc, that was rustfmt.
|
☀️ Test successful - status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For types that do not support derive Debug be implemented automatically by rust,
we know can generate implementations of the Debug trait. This code generation is
hidden behind the '--force-derive-debug' command-line flag.
Should solve: #875
Sorry for the extra noise in lib.rs, codegen/mod.rs etc, that was rustfmt.