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

Un-revert #51 and fix Windows build errors #55

Merged
merged 2 commits into from
May 3, 2023

Conversation

QuietMisdreavus
Copy link

Reverts #53

This brings back the upstream changes, effectively re-landing #51. I'm keeping this open while i look at the Windows CI failures that happened for the Swift compiler.

@QuietMisdreavus QuietMisdreavus changed the title Revert "Revert "merge in changes from cmark-gfm 0.29.0.gfm.11"" Un-revert #51 and fix Windows build errors Apr 28, 2023
@QuietMisdreavus
Copy link
Author

cc @compnerd; MSVC seems to be more stringent about symbol visibility mismatches than Clang. I couldn't even find an equivalent warning to enable in Clang to test this on macOS 😅

@compnerd
Copy link
Member

Yeah, MSVC is far better about bonking you over the head when you get things wrong. -Werror=inconsistent-dllimport-dllexport I think is the closest and its not nearly as good.

@QuietMisdreavus
Copy link
Author

The dllimport/dllexport attributes are only added on Windows, though, so there would need to be something for the equivalent __attribute__((__visibility__("default"))) (or "hidden") added on Mac/Linux. 🙃 Unfortunately, since the "exported" flag uses the default visibility, there's nothing to warn about when the attribute is missing. I tried adding -fvisibility=internal or something like that, but it didn't seem to affect anything.

@QuietMisdreavus
Copy link
Author

It looks like the cross-repo testing for Windows in swiftlang/swift#65518 worked, so this should be able to land without breaking the Swift compiler's build any more. 🤞

Copy link

@d-ronnqvist d-ronnqvist left a comment

Choose a reason for hiding this comment

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

These changes look good to me but I haven't tested them in a Windows build.

@QuietMisdreavus QuietMisdreavus merged commit 6c70377 into gfm May 3, 2023
@QuietMisdreavus QuietMisdreavus deleted the revert-53-revert-51-QuietMisdreavus/gfm.11 branch May 3, 2023 20:12
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.

3 participants