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

Convert config fields header, trailer, after_includes to text fields. #977

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

flaviojs
Copy link
Contributor

@flaviojs flaviojs commented Jun 18, 2024

Motivation:

Some cbindgen tests have a toml config file.
That config file must work with multiple languages.
Hard to understand tricks are used, making it harder for cbindgen contributors to make tests for their code.
A better solution is to allow language-specific text in text fields that allow "code".

Changes:

This PR attempts to do that, while maintaining backwards compatibility.
I chose a serde-compatible way to represent the data (untagged enums) but can change to whatever you want.

Only header, trailer, after_includes were changed.
Other fields that output raw "code" can be changed too.
No idea how docs.md should be changed so I left it alone.

Fields:

Text field can be a line field or a sequence of lines or structs:

field = [
   "sequence of lines",
   { language = "C", line = "or structs" },
]

Line field can be a single line or a struct:

field = "line"
field = { language = "C", line = "struct" }

#if 0
''' '
#endif
header = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem particularly an improvement fwiw. But after this patch, is there any test that remains with header = "string"? Does that still work with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you can still use strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Searching the codebase I found 1 test (swift_name) with header = "string" and 13 tests (asserted_cast/cfg_2/cfg/custom_header/deprecated/destructor_and_copy_ctor/layout_aligned_opaque/layout_packed_opaque/layout/must_use/nonnull_attribute/mod_attr/rename_crate) with

header = """
multi-line string
"""

@flaviojs
Copy link
Contributor Author

Hmm... line = "..." might be better as text = "..." since we use multi-line strings.
But then I don't know what to call the current Text enum.

@flaviojs
Copy link
Contributor Author

flaviojs commented Aug 17, 2024

I guess I should give a bit of background into how I arrived here.

While waiting for progress in #962 (volatile type qualifier) I attempted to fix other issues I have with cbindgen, like #43 (cyclical pointer references) and others.
During development some of the tests I modified kept failing and I didn't understand why. I tried reimplementing my code but that didn't help. Eventually I stopped and tried to analyze what was going on in depth and found that the problem was the weird code in the toml. After learning a bit of cython I finally understood what was going on.

Random example from pin.toml:

header = """
#if 0
''' '
#endif

#ifdef __cplusplus
template <typename T>
using Pin = T;
template <typename T>
using Box = T*;
#endif

#if 0
' '''
#endif
"""
  1. cython treats # as comments so those lines are ignored
  2. cython starts and ends multi-line strings with ''', turning the inner text into a string that can be ignored (the inner ' is probably to get meaningful errors in case of missmatching ''')
  3. C and C++ ignore the text inside the #if 0 blocks
  4. C ignores the text inside the #ifdef __cplusplus block

Basically, in this case the contents of header are supposed to affect only the C++ runs of the test, but I was forced to learn how C and C++ and cython treats this text to understand what was really going on.
When more backends are added this solution might break. <= PROBLEM
Incomprehensible errors frustrates people trying to contribute to the crate. <= PROBLEM

Ideally I would create an issue to get a discussing going so a solution could be ironed out, but it takes sooooo long to get any feedback in this repo and the code does not point to a particular structure or style to emulate, that I decided to just make a PR that can be tested and change whatever is requested.

A bunch of solutions were considered:

  1. what if there were keys for each language as well? it increases the maintenance burden and raises the question of which order should be printed (between generic and language specific)
  2. what if it was some kind of template? it increases the maintenance burden and raises the question of what should be available in the template
  3. what if the test code was annotated with the languages that it supports? this requires understanding the testing framework and needs feedback from the maintainers
  4. what if you could choose the target language of individual lines? the maintenance burden should be minimal, multi-line strings are already possible and you could interleave generic and language specific text
  5. ...(forgot the rest)

Cargo.toml allows the version of dependencies to be strings or tables, so I tried to replicate that kind of strategy with 4).
I ended up with a solution that is 100% backwards compatible and can be easily extended.
Templates can be added simply by implementing a different type of "line".

I'm not happy with the rust names so any feedback is appreciated.

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