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

Resolve NOMINMAX issues where possible #481

Merged
merged 4 commits into from
Dec 23, 2020
Merged

Resolve NOMINMAX issues where possible #481

merged 4 commits into from
Dec 23, 2020

Conversation

maxgolov
Copy link
Contributor

The issue is that Windows.h redefines both min and max as macros.

There are several fixes possible:

  • targeted fix: surround the usage of std::min and std::max with parenthesis; OR
  • globally define NOMINMAX for the entire project in CMake; OR
  • in those modules where it is known that std::* is going to be used, #define NOMINMAX right before the inclusion of Windows.h

Since there were two recent hits @mishal23 @jsuereth , I'm applying targeted fix to avoid this issue wherever possible.

Not defining NOMINMAX globally, as this may lead to other interesting side-effects inside the bowels of Windows SDK 😁

@maxgolov maxgolov requested review from a team and jsuereth December 22, 2020 19:47
# ifndef NOMINMAX
# define NOMINMAX
# endif
# include <Windows.h>
Copy link
Contributor

@ThomsonTan ThomsonTan Dec 22, 2020

Choose a reason for hiding this comment

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

EDIT, sorry for the wrong paste. I was asking how could if the user could include windows.h before include our header file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry Tom, what do you mean by C:\GH\otcpp\ext\include\opentelemetry\ext\http\client\curl\http_operation_curl.h(208) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reason why I'm adding this is that Josh would need this in his PR that is failing. Related PR: #443

Copy link
Contributor Author

@maxgolov maxgolov Dec 22, 2020

Choose a reason for hiding this comment

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

if the user could include windows.h before include our header file

Yeah, then it will break for those users. We most likely need to add some blurb in our documentation, that since our SDK uses range limits extensively in public API, that means we should tell users that they must revisit their usage of min and max macros, to make sure we are not breaking them on Windows..

Basically then the user should add the NOMINMAX define on their end ..

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 tried fixing this where possible. But there are some spots, where our SDK defines class methods called min and max - that will surely break if our Windows customers don't define NOMINMAX themselves for their project.

@codecov
Copy link

codecov bot commented Dec 22, 2020

Codecov Report

Merging #481 (0d705e1) into master (2c97611) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
+ Coverage   94.36%   94.38%   +0.02%     
==========================================
  Files         189      189              
  Lines        8410     8410              
==========================================
+ Hits         7936     7938       +2     
+ Misses        474      472       -2     
Impacted Files Coverage Δ
api/include/opentelemetry/common/spin_lock_mutex.h 33.33% <ø> (ø)
sdk/src/common/fast_random_number_generator.h 100.00% <ø> (ø)
api/include/opentelemetry/nostd/string_view.h 97.56% <100.00%> (ø)
...lemetry/sdk/metrics/aggregator/sketch_aggregator.h 69.76% <100.00%> (ø)
sdk/src/logs/batch_log_processor.cc 95.06% <0.00%> (+1.23%) ⬆️
sdk/test/metrics/counter_aggregator_test.cc 100.00% <0.00%> (+1.78%) ⬆️

@lalitb lalitb merged commit 25a8668 into master Dec 23, 2020
@maxgolov maxgolov deleted the maxgolov/nominmax branch December 24, 2020 00:16
kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Dec 31, 2020
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.

4 participants