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

C++: Improve error message when finding X11 macro Unsorted #7855

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

emilk
Copy link
Member

@emilk emilk commented Oct 21, 2024

Some C library uses macros for constants, which makes those identifiers unusable by any other library in the same global namespace. So anyone including both X.h and rerun.hpp in the same compilation unit (in that order) would have problems. It's a dumpster-fire.

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!
  • If have noted any breaking changes to the log API in CHANGELOG.md and the migration guide

To run all checks from main, comment on the PR with @rerun-bot full-check.

@emilk emilk added 🪳 bug Something isn't working 🌊 C++ API C/C++ API specific include in changelog labels Oct 21, 2024
@rgolovanov
Copy link
Contributor

@emilk , @Wumpf
We probably shouldn't do it inside the rerun's header. I would leave it to the end user to decide how to deal with this. Maybe, maximum, what rerun could do is to show #pragma warning explaining the case to the user and the user would need to do undef/define properly.

In current solution, if you #undef Unsorted then it may potentially break something else for the user...

@emilk
Copy link
Member Author

emilk commented Oct 22, 2024

Something like

#ifdef Unsorted
#    error "There is a conflicting macro 'Unsorted' (probably from X11). Either include <rerun.hpp> FIRST, OR add '#undef Unsorted' before '#include <rerun.hpp>"
#endif

?

@Wumpf
Copy link
Member

Wumpf commented Oct 22, 2024

I think I have a slight preference for the error message, as rgolovanov says otherwise we might break a user unexpectedly.

Either include <rerun.hpp> FIRST,

that part is not a good suggestion because it means that if you only do that you'll still break when trying to use rerun::SortingStatus::Unsorted. Yes, it will compile as long as you don't use it, but it just breaks later then in your code

I'd suggest:

#ifdef Unsorted
#    error "There is a macro 'Unsorted' (probably from X11) defined, incorrectly replacing one of the variants of `rerun::SortingStatus`. Add '#undef Unsorted' before '#include <rerun.hpp>' to work around this."
#endif

that then essentially only assumes that the user will know what happens if they get an error for Unsorted not being known to the compiler down the line (which is fairly unlikely, but who knows!)

@emilk emilk changed the title Fix conflicts with X11 headers by adding #undef Unsorted C++: Improve error message when finding X11 macro Unsorted Oct 22, 2024
@emilk emilk requested a review from Wumpf October 22, 2024 09:30
@Wumpf Wumpf added this to the Next patch release milestone Oct 22, 2024
@Wumpf Wumpf merged commit 222a679 into main Oct 22, 2024
36 checks passed
@Wumpf Wumpf deleted the emilk/undef-unsorted branch October 22, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 🌊 C++ API C/C++ API specific include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enum SortingStatus::Unsorted conflicts with define in X.h
3 participants