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

fix: rename typename U to typename UT to avoid cpprest conflicts #4186

Closed
wants to merge 1 commit into from

Conversation

aminya
Copy link
Contributor

@aminya aminya commented Oct 2, 2024

Fixes #4180
Workaround for microsoft/cpprestsdk#1805

FMT_CONSTEXPR auto compare(const Char* s1, const Char* s2, std::size_t n)
-> int {
FMT_CONSTEXPR auto compare(const Char* s1, const Char* s2,
std::size_t n) -> int {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The formatting changes are caused by clang-format so that CI doesn't fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using clang-format-18. Using 17 reduces the diff

@vitaut
Copy link
Contributor

vitaut commented Oct 2, 2024

Thanks for the PR but we shouldn't clutter the code because of poorly written third-party libraries.

@vitaut vitaut closed this Oct 2, 2024
@aminya
Copy link
Contributor Author

aminya commented Oct 2, 2024

Thanks for the PR but we shouldn't clutter the code because of poorly written third-party libraries.

That's fair. However, unfortunately, I noticed that OpenAPI cpprest generator also uses U. I don't think it's viable for cpprest to remove the macro at this point. I am going to patch OpenAPI generator to undef U at the end of the headers. A lot of work for a bad name.

https://github.com/OpenAPITools/openapi-generator/blob/673cd15e72d8dc110bcb685a36a4e0b4c8dfe67c/modules/openapi-generator/src/main/resources/cpp-rest-sdk-client/model-source.mustache#L83

@edo9300
Copy link
Contributor

edo9300 commented Oct 4, 2024

That macro is only replacing U() calls, a maybe less invasive change could be to replace them with uniform initializations U{}

@aminya
Copy link
Contributor Author

aminya commented Oct 4, 2024

@vitaut @edo9300 The actual Diff is much smaller than in the PR:
master...aminya:fmt:type-conficlit

I was still fixing the clang-format version when the PR was closed.

Also, in general using readable typenames instead of single characters is a good idea, however that's another discussion

@edo9300
Copy link
Contributor

edo9300 commented Oct 4, 2024

Actually, the conflicting code isn't present at all in the current master, so the workaround isn't needed in first place

@edo9300
Copy link
Contributor

edo9300 commented Oct 4, 2024

Removed in 761d35f

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.

fmt 11 cannot format string_view
3 participants