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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
daee5ae
Resolve NOMINMAX issues where possible
maxgolov Dec 22, 2020
354efb5
Fix formatting issue
maxgolov Dec 22, 2020
9157954
More NOMINMAX fixes
maxgolov Dec 22, 2020
0d705e1
Merge branch 'master' into maxgolov/nominmax
lalitb Dec 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions api/include/opentelemetry/common/spin_lock_mutex.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@

#include "opentelemetry/version.h"

#if defined(_MSC_VER)
# ifndef NOMINMAX
# define NOMINMAX
# endif
# include <Windows.h>
Copy link
Contributor

@ThomsonTan ThomsonTan Dec 22, 2020 •
edited
Loading

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 •
edited
Loading

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.

#elif defined(__i386__) || defined(__x86_64__)
# if defined(__clang__)
# include <emmintrin.h>
# endif
#endif

OPENTELEMETRY_BEGIN_NAMESPACE
namespace common
{
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/nostd/string_view.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ class string_view

int compare(string_view v) const noexcept
{
size_type len = std::min(size(), v.size());
size_type len = (std::min)(size(), v.size());
int result = Traits::compare(data(), v.data(), len);
if (result == 0)
result = size() == v.size() ? 0 : (size() < v.size() ? -1 : 1);
Expand Down
7 changes: 5 additions & 2 deletions api/include/opentelemetry/plugin/detail/dynamic_load_windows.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
#include "opentelemetry/plugin/hook.h"
#include "opentelemetry/version.h"

#include <windows.h>
#ifndef NOMINMAX
# define NOMINMAX
#endif
#include <Windows.h>

#include <WinBase.h>
#include <errhandlingapi.h>
#include <winbase.h>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace plugin
Expand Down
2 changes: 1 addition & 1 deletion api/include/opentelemetry/std/span.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ OPENTELEMETRY_END_NAMESPACE
OPENTELEMETRY_BEGIN_NAMESPACE
namespace nostd
{
constexpr std::size_t dynamic_extent = std::numeric_limits<std::size_t>::max();
constexpr std::size_t dynamic_extent = (std::numeric_limits<std::size_t>::max());

template <class ElementType, std::size_t Extent = nostd::dynamic_extent>
using span = std::span<ElementType, Extent>;
Expand Down
2 changes: 1 addition & 1 deletion ext/include/opentelemetry/ext/http/server/socket_tools.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ struct SocketAddr
{
inet4.sin_port = htons(atoi(colon + 1));
char buf[16];
memcpy(buf, addr, std::min<ptrdiff_t>(15, colon - addr));
memcpy(buf, addr, (std::min<ptrdiff_t>)(15, colon - addr));
buf[15] = '\0';
::inet_pton(AF_INET, buf, &inet4.sin_addr);
}
Expand Down
2 changes: 1 addition & 1 deletion sdk/include/opentelemetry/sdk/metrics/aggregator/sketch_aggregator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ class SketchAggregator final : public Aggregator<T>
int idx;
if (val == 0)
{
idx = std::numeric_limits<int>::min();
idx = (std::numeric_limits<int>::min());
}
else
{
Expand Down
11 changes: 11 additions & 0 deletions sdk/src/common/fast_random_number_generator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ namespace common
* std::mt19937_64; and since we don't care about the other beneficial random
* number properties that std:mt19937_64 provides for this application, it's a
* entirely appropriate replacement.
*
* Note for Windows users - please make sure that NOMINMAX is defined, e.g.
*
* ...
* #define NOMINMAX
* #include <Windows.h>
* ...
*
* See:
* https://stackoverflow.com/questions/13416418/define-nominmax-using-stdmin-max
*
*/
class FastRandomNumberGenerator
{
Expand Down