Skip to content

Windows: test compilation refresh#11719

Merged
asraa merged 2 commits intoenvoyproxy:masterfrom
greenhouse-org:windows-test-compilation-refresh
Jun 25, 2020
Merged

Windows: test compilation refresh#11719
asraa merged 2 commits intoenvoyproxy:masterfrom
greenhouse-org:windows-test-compilation-refresh

Conversation

@sunjayBhatia
Copy link
Member

@sunjayBhatia sunjayBhatia commented Jun 23, 2020

Commit Message:
Windows: test compilation refresh

Additional Description: N/A
Risk Level: Low
Testing: Fixes test compilation on Windows, should be a no-op for other platforms
Docs Changes: N/A
Release Notes: N/A

- Remove TestHeaderMapImplBase constructor that takes a list of
std::pair<LowerCaseString, std::string>
  - MSVC erroneously treats this as ambiguous. It appears the explicit
  attribute of the LowerCaseString constructor is incorrectly discarded.
  We are able to reproduce this in a minimal example and see that MSVC is wrong
  here: https://godbolt.org/z/VjgsAi
  - To mitigate, remove LowerCaseString based constructor since it is
  only used in tests, tests always use std::string
- Correct use of long in envoy_quic_alarm.cc, explicitly cast to int64_t
(long on Windows is 32 bits)
- `using std::string_literal::operator""s` is erroneously rejected by
MSVC, throwing error C4455.
  - Instead, simply utilize the namespace and continue to use the
  operator as is
  - operator usage could be replaced by `std::string (const char* s,
  size_t n)` constructor
  - See https://developercommunity.visualstudio.com/content/problem/270349/warning-c4455-issued-when-using-standardized-liter.html
  and other related duplicate issues that have not yet been resolved.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>
}
header_map_->verifyByteSizeInternalForTest();
}
TestHeaderMapImplBase(
Copy link
Member Author

Choose a reason for hiding this comment

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

This was introduced by #11546 cc @mattklein123 in case you have more context/if we should not remove this, the PR description has a godbolt.org link that shows the same issue we see on Windows

@sunjayBhatia
Copy link
Member Author

Blocks PR #11107

@sunjayBhatia
Copy link
Member Author

cc @wrowe @davinci26 @nigriMSFT

- yaml-cpp parser should be treating cr+lf as newline per spec

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>
@wrowe
Copy link
Contributor

wrowe commented Jun 23, 2020

Local testing indicates this passes on Windows for //test/... addressing all regressions over the past week. So this is ready for review, and in combination with #11107 the CI for windows will incorporate RBE including testing.

@asraa asraa merged commit c4a2189 into envoyproxy:master Jun 25, 2020
chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Jun 25, 2020
* Windows: test compilation refresh
- Remove TestHeaderMapImplBase constructor that takes a list of
std::pair<LowerCaseString, std::string>
  - MSVC erroneously treats this as ambiguous. It appears the explicit
  attribute of the LowerCaseString constructor is incorrectly discarded.
  We are able to reproduce this in a minimal example and see that MSVC is wrong
  here: https://godbolt.org/z/VjgsAi
  - To mitigate, remove LowerCaseString based constructor since it is
  only used in tests, tests always use std::string
- Correct use of long in envoy_quic_alarm.cc, explicitly cast to int64_t
(long on Windows is 32 bits)
- `using std::string_literal::operator""s` is erroneously rejected by
MSVC, throwing error C4455.
  - Instead, simply utilize the namespace and continue to use the
  operator as is
  - operator usage could be replaced by `std::string (const char* s,
  size_t n)` constructor
  - See https://developercommunity.visualstudio.com/content/problem/270349/warning-c4455-issued-when-using-standardized-liter.html
  and other related duplicate issues that have not yet been resolved.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>
Signed-off-by: chaoqinli <chaoqinli@google.com>
songhu pushed a commit to songhu/envoy that referenced this pull request Jun 25, 2020
* Windows: test compilation refresh
- Remove TestHeaderMapImplBase constructor that takes a list of
std::pair<LowerCaseString, std::string>
  - MSVC erroneously treats this as ambiguous. It appears the explicit
  attribute of the LowerCaseString constructor is incorrectly discarded.
  We are able to reproduce this in a minimal example and see that MSVC is wrong
  here: https://godbolt.org/z/VjgsAi
  - To mitigate, remove LowerCaseString based constructor since it is
  only used in tests, tests always use std::string
- Correct use of long in envoy_quic_alarm.cc, explicitly cast to int64_t
(long on Windows is 32 bits)
- `using std::string_literal::operator""s` is erroneously rejected by
MSVC, throwing error C4455.
  - Instead, simply utilize the namespace and continue to use the
  operator as is
  - operator usage could be replaced by `std::string (const char* s,
  size_t n)` constructor
  - See https://developercommunity.visualstudio.com/content/problem/270349/warning-c4455-issued-when-using-standardized-liter.html
  and other related duplicate issues that have not yet been resolved.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>
@sunjayBhatia sunjayBhatia deleted the windows-test-compilation-refresh branch June 30, 2020 16:05
yashwant121 pushed a commit to yashwant121/envoy that referenced this pull request Jul 24, 2020
* Windows: test compilation refresh
- Remove TestHeaderMapImplBase constructor that takes a list of
std::pair<LowerCaseString, std::string>
  - MSVC erroneously treats this as ambiguous. It appears the explicit
  attribute of the LowerCaseString constructor is incorrectly discarded.
  We are able to reproduce this in a minimal example and see that MSVC is wrong
  here: https://godbolt.org/z/VjgsAi
  - To mitigate, remove LowerCaseString based constructor since it is
  only used in tests, tests always use std::string
- Correct use of long in envoy_quic_alarm.cc, explicitly cast to int64_t
(long on Windows is 32 bits)
- `using std::string_literal::operator""s` is erroneously rejected by
MSVC, throwing error C4455.
  - Instead, simply utilize the namespace and continue to use the
  operator as is
  - operator usage could be replaced by `std::string (const char* s,
  size_t n)` constructor
  - See https://developercommunity.visualstudio.com/content/problem/270349/warning-c4455-issued-when-using-standardized-liter.html
  and other related duplicate issues that have not yet been resolved.

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>
Co-authored-by: Sunjay Bhatia <sunjayb@vmware.com>
Signed-off-by: William A Rowe Jr <wrowe@vmware.com>
Co-authored-by: William A Rowe Jr <wrowe@vmware.com>
Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>
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