Skip to content

tests: Remove Cython-specific configs#620

Merged
emilio merged 1 commit intomozilla:masterfrom
petrochenkov:dupconf
Nov 25, 2020
Merged

tests: Remove Cython-specific configs#620
emilio merged 1 commit intomozilla:masterfrom
petrochenkov:dupconf

Conversation

@petrochenkov
Copy link
Copy Markdown
Contributor

@petrochenkov petrochenkov commented Nov 18, 2020 •
edited
Loading

This is an alternative to #618 that doesn't require running preprocessor or concatenating configs. Cython-specific configs are removed using a combination of several changes.

  • Removing Cython-specific code from C/C++ by putting it into #if 0. (Anything starting with # is a comment in Cython, and doesn't need to be removed.) 
    #if 0

Cython-specific

#endif
  • Removing C/C++-specific code from Cython by putting it into multi-line comments (Python docstrings)... and removing the docstring quotes from C/C++, not very pretty. 
    #if 0
''' '
#endif

C/C++-specific

#if 0
' '''
#endif
  • Tweaking tests in insignificant ways (changing indents, removing insignificant comments).
  • Tweaking tests in significant ways (simplifying constants that crash Cython compiler, only 2 tests).
  • Some actual improvements to cbindgen logic allowing it to not generate invalid code.

@petrochenkov petrochenkov mentioned this pull request Nov 18, 2020
Closed
emilio
emilio approved these changes Nov 25, 2020
View reviewed changes
Copy link
Copy Markdown
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

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

I hate it / love it :-)

I think this is fine. A bit unfortunate to have to comment out some tests... Do you know if these bugs are filed upstream?

src/bindgen/ir/enumeration.rs
fn write<F: Write>(&self, config: &Config, out: &mut SourceWriter<F>) {
let condition = self.cfg.to_condition(config);
condition.write_before(config, out);
if config.language != Language::Cython {
Copy link
Copy Markdown
Collaborator

@emilio emilio Nov 18, 2020

Choose a reason for hiding this comment

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

Maybe add a comment like "cython doesn't support conditional enum variants" or such?

@emilio emilio merged commit 1c1d475 into mozilla:master Nov 25, 2020
@petrochenkov
Copy link
Copy Markdown
Contributor Author

petrochenkov commented Nov 25, 2020

Do you know if these bugs are filed upstream?

I'll try to find out, and report if they are not yet reported.

@emilio
Copy link
Copy Markdown
Collaborator

emilio commented Nov 25, 2020

Thanks!

@petrochenkov petrochenkov mentioned this pull request Nov 25, 2020
Merged
@petrochenkov
Copy link
Copy Markdown
Contributor Author

petrochenkov commented Nov 25, 2020

Reported in cython/cython#3918.

This was referenced Nov 26, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emilio emilio emilio approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@petrochenkov @emilio