Apply defines to struct literal #956
Conversation
youknowone
force-pushed
the
cfg-literal
branch
3 times, most recently
from
cc3bb3f
to
0e7afe1
Compare
tests/rust/cfg_field.rs
Outdated
| #[repr(C)] | ||
| pub struct Bar { | ||
| pub x: i32, | ||
| #[cfg(windows)] | ||
| pub y: u32, | ||
| } | ||
|
|
||
| impl Bar { | ||
| pub const BAR: Self = Self { | ||
| x: 0, | ||
| #[cfg(windows)] | ||
| y: 0, | ||
| }; | ||
| } |
There was a problem hiding this comment.
How can I properly add a test? This change doesn't add diff
|
I didn't push newline/indent fix not to break test yet. Please let me know if the approach is okay. Then I will finish the whitespace work. |
| // In C++, same order as defined is required. | ||
| let ordered_fields = out.bindings().struct_field_names(path); | ||
| for ordered_key in ordered_fields.iter() { | ||
| let last_i = ordered_fields.len() - 1; |
There was a problem hiding this comment.
Ah, the question didn't get saved. It's basically... can't this underflow? It'd be better to do if ordered_fields.is_empty() { 0 } else { ordered_fields.len() - 1 };
There was a problem hiding this comment.
Ah, it might be harder than that... Recording with a bit whether we have conditional fields, and either skip generating the constant or generate it without the conditions for C seems feasible.
Alternatively doing some macro soup for C might work but it is annoying. Something like gathering the conditional fields first, and generating something like:
#ifdef X11
#define zero_(v) .zero = v,
#else
#define zero_(v)
#endif
#define ConditionalField_ZERO (ConditionalField){ \
zero_(0) \
};
Or so, but that gets clunky.
youknowone
force-pushed
the
cfg-literal
branch
from
41d3ce9
to
dd3faf9
Compare
There was a problem hiding this comment.
Thank you for the advice. It had progress, but not yet finished.
I left another comment for duplicated field generation.
Another problem is constexpr context. This is solvedwrite_literal needs context about allow_constexpr to choose macro or inline field. Do you have a suggestion for that?
| if self.config.language == Language::C { | ||
| for ordered_key in ordered_fields.iter() { | ||
| if let Some(lit) = fields.get(ordered_key) { | ||
| if let Some(condition) = lit.cfg.to_condition(self.config) { | ||
| out.new_line(); | ||
| condition.write_before(self.config, out); | ||
| let define = format!("#define __{export_name}_{ordered_key}(v)"); | ||
| write!(out, "{define} (v)"); | ||
| write!(out, "\n#else\n"); | ||
| write!(out, "{define}"); | ||
| condition.write_after(self.config, out); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Where is the good place to put this?
From this place, it will write duplicated definitions for each literal definition. That will trigger compiler warnings about redefinition.
There was a problem hiding this comment.
@emilio This part is also not resolved yet
youknowone
force-pushed
the
cfg-literal
branch
from
dd3faf9
to
4d06178
Compare
| @@ -911,30 +911,42 @@ impl LanguageBackend for CLikeLanguageBackend<'_> { | |||
| fields, | |||
| path, | |||
| } => { | |||
| let is_constexpr = self.config.language == Language::Cxx | |||
| && (self.config.constant.allow_constexpr || l.can_be_constexpr()); | |||
There was a problem hiding this comment.
This seems wrong? Shouldn't it be allow_constexpr && can_be_constexpr?
There was a problem hiding this comment.
Oh.. I was confused by allow_static_const. Thank you
| if is_constexpr { | ||
| if i > 0 { | ||
| write!(out, ", "); | ||
| } |
There was a problem hiding this comment.
Factor this out of the if/else?
There was a problem hiding this comment.
If you looked in first commit, this is changed in second commit
There was a problem hiding this comment.
If you don't mind, I separated the former 2 commits to another PR in #988, which is not directly related to cfg handling.
| } else { | ||
| write!(out, ".{} = ", ordered_key); | ||
| } | ||
| self.write_literal(out, lit); |
tests/expectations/cfg_both.c
Outdated
| #else | ||
| #define __ConditionalField_field(v) | ||
| #endif | ||
| #define ConditionalField_ONE (ConditionalField){ .field = __ConditionalField_field(1) } |
There was a problem hiding this comment.
Does this even build when it's not defined? Doesn't it expand to .field =?
There was a problem hiding this comment.
Thanks, fixed it
youknowone
force-pushed
the
cfg-literal
branch
from
4d06178
to
cfa7364
Compare
|
@emilio could you take another look? |
youknowone
force-pushed
the
cfg-literal
branch
from
cfa7364
to
22a4c6f
Compare
Fix #955