Skip to content

Comments

Make on TextLogLevel PascalCase (instead of SCREAMING CASE) to avoid clashes with preprocessor defines#4152

Merged
Wumpf merged 6 commits intomainfrom
andreas/cpp/fix-potential-preprocessor-clash
Nov 6, 2023
Merged

Make on TextLogLevel PascalCase (instead of SCREAMING CASE) to avoid clashes with preprocessor defines#4152
Wumpf merged 6 commits intomainfrom
andreas/cpp/fix-potential-preprocessor-clash

Conversation

@Wumpf
Copy link
Member

@Wumpf Wumpf commented Nov 6, 2023

What

Considered also just using lower case on these (nobody would do #define debug, ... RIGHT?). But they are enum - like constants so all-caps felt appropriate.

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 demo.rerun.io (if applicable)
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@Wumpf Wumpf added 🪳 bug Something isn't working sdk-cpp C/C++ API specific labels Nov 6, 2023
Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

That was my preferred choice, so I'm gonna approve this ;)

@emilk
Copy link
Member

emilk commented Nov 6, 2023

Alternative: use PascalCase for constants in C++. See #4151 (comment)

@Wumpf
Copy link
Member Author

Wumpf commented Nov 6, 2023

Discussed this a bit more on internal Slack: there's some risk that Debug is a define as well, but we'll take that.
Our enums that are emitted for arrow union types already follow this convention so we can stick with that everywhere now

@Wumpf Wumpf changed the title Prefix constants on TextLogLevel with LEVEL_ to avoid clashes with preprocessor defines Make on TextLogLevel PascalCase (instead of SCREAMING CASE) to avoid clashes with preprocessor defines Nov 6, 2023
@Wumpf Wumpf requested a review from abey79 November 6, 2023 17:05
Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

lgtm

@Wumpf Wumpf merged commit 8a97416 into main Nov 6, 2023
@Wumpf Wumpf deleted the andreas/cpp/fix-potential-preprocessor-clash branch November 6, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🪳 bug Something isn't working include in changelog sdk-cpp C/C++ API specific

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C++ API, DEBUG symbol conflict

3 participants