Apply defines to struct literal by youknowone · Pull Request #956 · mozilla/cbindgen

youknowone · 2024-05-17T07:49:08Z

youknowone · 2024-05-17T10:25:50Z

tests/rust/cfg_field.rs

+#[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,
+    };
+}


How can I properly add a test? This change doesn't add diff

youknowone · 2024-05-17T10:26:56Z

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.

emilio

Thanks, looks good with a question.

Regarding the test, let's put it in tests/rust/cfg.rs.

Feel free to remove tests/rust/cfg_field.rs, it was meant as a crashtest at some point, looking at the git history, but right now it's covered by tests/rust/cfg.

emilio · 2024-05-26T15:53:18Z

src/bindgen/language_backend/clike.rs

                // 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;


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 };

tests/expectations/cfg.compat.c

emilio

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.

tests/expectations/cfg.compat.c

youknowone

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. write_literal needs context about allow_constexpr to choose macro or inline field. Do you have a suggestion for that?~~ This is solved

youknowone · 2024-08-10T14:37:36Z

src/bindgen/language_backend/clike.rs

+                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);
+                            }
+                        }
+                    }
+                }


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.

@emilio This part is also not resolved yet

emilio · 2024-08-11T15:15:53Z

src/bindgen/language_backend/clike.rs

                path,
            } => {
+                let is_constexpr = self.config.language == Language::Cxx
+                    && (self.config.constant.allow_constexpr || l.can_be_constexpr());


This seems wrong? Shouldn't it be allow_constexpr && can_be_constexpr?

Oh.. I was confused by allow_static_const. Thank you

emilio · 2024-08-11T15:16:31Z

src/bindgen/language_backend/clike.rs

+                        if is_constexpr {
+                            if i > 0 {
+                                write!(out, ", ");
+                            }


Factor this out of the if/else?

If you looked in first commit, this is changed in second commit

If you don't mind, I separated the former 2 commits to another PR in #988, which is not directly related to cfg handling.

emilio · 2024-08-11T15:16:59Z

src/bindgen/language_backend/clike.rs

+                            } else {
+                                write!(out, ".{} = ", ordered_key);
+                            }
+                            self.write_literal(out, lit);


emilio · 2024-08-11T15:21:23Z

tests/expectations/cfg_both.c

+#else
+#define __ConditionalField_field(v)
+#endif
+#define ConditionalField_ONE (ConditionalField){ .field = __ConditionalField_field(1) }


Does this even build when it's not defined? Doesn't it expand to .field =?

Thanks, fixed it

youknowone · 2024-09-24T12:46:05Z

@emilio could you take another look?

youknowone · 2025-05-20T12:19:58Z

Though #989 fixed the similar case to #955 for modern C++ using constexpr, this patch using #define is not finished yet.

I am not interested in finishing this PR anymore because #989 is enough for my case. Please feel free to take it if anyone is interested in it.

youknowone force-pushed the cfg-literal branch 3 times, most recently from cc3bb3f to 0e7afe1 Compare May 17, 2024 10:25

youknowone commented May 17, 2024

View reviewed changes

youknowone force-pushed the cfg-literal branch from 0e7afe1 to 44e3fca Compare May 21, 2024 02:23

emilio requested changes May 26, 2024

View reviewed changes

emilio reviewed May 26, 2024

View reviewed changes

youknowone force-pushed the cfg-literal branch from 44e3fca to 41d3ce9 Compare May 27, 2024 20:59

youknowone commented May 27, 2024

View reviewed changes

tests/expectations/cfg.compat.c Show resolved Hide resolved

emilio requested changes Aug 10, 2024

View reviewed changes

tests/expectations/cfg.compat.c Show resolved Hide resolved

youknowone force-pushed the cfg-literal branch from 41d3ce9 to dd3faf9 Compare August 10, 2024 14:35

youknowone commented Aug 10, 2024

View reviewed changes

youknowone mentioned this pull request Aug 10, 2024

Newline for each field for constexpr field constants #988

Merged

youknowone force-pushed the cfg-literal branch from dd3faf9 to 4d06178 Compare August 10, 2024 16:07

emilio reviewed Aug 11, 2024

View reviewed changes

youknowone force-pushed the cfg-literal branch from 4d06178 to cfa7364 Compare August 12, 2024 12:41

youknowone force-pushed the cfg-literal branch from cfa7364 to 22a4c6f Compare October 29, 2024 08:01

youknowone added 2 commits May 20, 2025 21:13

ConditionWrite for Condition

8b6e938

struct literal defines by condition

d3a5c70

youknowone force-pushed the cfg-literal branch from 22a4c6f to d3a5c70 Compare May 20, 2025 12:15

Conversation

youknowone commented May 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented May 17, 2024

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone Aug 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone commented Sep 24, 2024

Uh oh!

youknowone commented May 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

youknowone left a comment •

edited

Loading

youknowone Aug 10, 2024 •

edited

Loading