Skip to content

Cleaning up some warnings for Clang and GCC #1255

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

Merged
merged 13 commits into from
Jan 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions strings/base_activation.h
Original file line number Diff line number Diff line change
Expand Up @@ -196,8 +196,10 @@ namespace winrt::impl

#ifdef _WIN64
inline constexpr uint32_t memory_allocation_alignment{ 16 };
#ifdef _MSC_VER
#pragma warning(push)
#pragma warning(disable:4324) // structure was padded due to alignment specifier
#endif
struct alignas(16) slist_entry
{
slist_entry* next;
Expand All @@ -217,7 +219,9 @@ namespace winrt::impl
uint64_t reserved4 : 60;
} reserved2;
};
#ifdef _MSC_VER
#pragma warning(pop)
#endif
#else
inline constexpr uint32_t memory_allocation_alignment{ 8 };
struct slist_entry
Expand Down
29 changes: 22 additions & 7 deletions strings/base_array.h
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,26 @@ WINRT_EXPORT namespace winrt
this->m_size = size;
}
}

std::pair<uint32_t, impl::arg_out<T>> detach_abi() noexcept
{
#ifdef _MSC_VER
// https://github.com/microsoft/cppwinrt/pull/1165
std::pair<uint32_t, impl::arg_out<T>> result;
memset(&result, 0, sizeof(result));
result.first = this->size();
result.second = *reinterpret_cast<impl::arg_out<T>*>(this);
memset(this, 0, sizeof(com_array<T>));
#else
std::pair<uint32_t, impl::arg_out<T>> result(this->size(), *reinterpret_cast<impl::arg_out<T>*>(this));
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this reintroduce the uninitialized padding data for non-VC compilers, per the comments in the PR linked above? What compiler warning does this resolve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GCC warning is 'void* memset(void*, int, size_t)' clearing an object of type 'struct std::pair<unsigned int, float*>' with no trivial copy-assignment; use assignment or value-initialization instead [-Wclass-memaccess].

This change does leave the padding uninitialized, but as said before it is "almost never going to be an issue for this function" and I am uncomfortable with the attempt to fix it potentially invoking UB. (Though I am no language lawyer so I cannot say for sure whether there is UB.)

this->m_data = nullptr;
this->m_size = 0;
#endif
return result;
}

template <typename U>
friend std::pair<uint32_t, impl::arg_out<U>> detach_abi(com_array<U>& object) noexcept;
};

template <typename C> com_array(uint32_t, C const&) -> com_array<std::decay_t<C>>;
Expand Down Expand Up @@ -418,14 +438,9 @@ WINRT_EXPORT namespace winrt
}

template <typename T>
auto detach_abi(com_array<T>& object) noexcept
std::pair<uint32_t, impl::arg_out<T>> detach_abi(com_array<T>& object) noexcept
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above. This change undoes an intentional change made to address a customer-reported issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you recall the issue? We should have a test to avoid such a regression, but all tests pass on this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, you're referring to #1165 - that was itself to suppress a compiler warning. 🙄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, we can avoid the memset in #1165 and just suppress the code analysis warning but I don't know whether that's possible since its presumably not an actual compiler warning. Perhaps @DefaultRyan can chime in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks to me like the original change was to suppress a warning in MSVC and this change is to suppress a warning in GCC. So if nobody objects, I'll go ahead and approve this change.

{
std::pair<uint32_t, impl::arg_out<T>> result;
memset(&result, 0, sizeof(result));
result.first = object.size();
result.second = *reinterpret_cast<impl::arg_out<T>*>(&object);
memset(&object, 0, sizeof(com_array<T>));
return result;
return object.detach_abi();
}

template <typename T>
Expand Down
2 changes: 2 additions & 0 deletions strings/base_events.h
Original file line number Diff line number Diff line change
Expand Up @@ -331,7 +331,9 @@ namespace winrt::impl
com_ptr<event_array<T>> make_event_array(uint32_t const capacity)
{
void* raw = ::operator new(sizeof(event_array<T>) + (sizeof(T)* capacity));
#ifdef _MSC_VER
#pragma warning(suppress: 6386)
#endif
return { new(raw) event_array<T>(capacity), take_ownership_from_abi };
}

Expand Down
4 changes: 4 additions & 0 deletions strings/base_identity.h
Original file line number Diff line number Diff line change
Expand Up @@ -453,12 +453,16 @@ namespace winrt::impl
template <typename T>
struct pinterface_guid
{
#ifdef _MSC_VER
#pragma warning(suppress: 4307)
#endif
static constexpr guid value{ generate_guid(signature<T>::data) };
};

template <typename T>
#ifdef _MSC_VER
#pragma warning(suppress: 4307)
#endif
inline constexpr auto name_v
{
combine
Expand Down
9 changes: 9 additions & 0 deletions strings/base_implements.h
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,9 @@ namespace winrt::impl
template <typename ... T>
struct uncloaked_iids<interface_list<T...>>
{
#ifdef _MSC_VER
#pragma warning(suppress: 4307)
#endif
static constexpr std::array<guid, sizeof...(T)> value{ winrt::guid_of<T>() ... };
};

Expand Down Expand Up @@ -1462,6 +1464,10 @@ WINRT_EXPORT namespace winrt
return result;
}

#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Winconsistent-missing-override"
#endif
impl::hresult_type __stdcall QueryInterface(impl::guid_type const& id, void** object) noexcept
{
return root_implements_type::QueryInterface(reinterpret_cast<guid const&>(id), object);
Expand Down Expand Up @@ -1493,6 +1499,9 @@ WINRT_EXPORT namespace winrt
{
return root_implements_type::abi_GetTrustLevel(reinterpret_cast<Windows::Foundation::TrustLevel*>(value));
}
#ifdef __clang__
#pragma clang diagnostic pop
#endif

void* find_interface(guid const& id) const noexcept override
{
Expand Down
6 changes: 6 additions & 0 deletions strings/base_macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@

#define WINRT_IMPL_SHIM(...) (*(abi_t<__VA_ARGS__>**)&static_cast<__VA_ARGS__ const&>(static_cast<D const&>(*this)))

#ifdef _MSC_VER
// Note: this is a workaround for a false-positive warning produced by the Visual C++ 15.9 compiler.
#pragma warning(disable : 5046)

// Note: this is a workaround for a false-positive warning produced by the Visual C++ 16.3 compiler.
#pragma warning(disable : 4268)
#endif

#if defined(__cpp_lib_coroutine) || defined(__cpp_coroutines) || defined(_RESUMABLE_FUNCTIONS_SUPPORTED)
#define WINRT_IMPL_COROUTINES
Expand Down Expand Up @@ -87,7 +89,9 @@ typedef struct _GUID GUID;
#define WINRT_IMPL_SOURCE_LOCATION_FORWARD , sourceInformation
#define WINRT_IMPL_SOURCE_LOCATION_FORWARD_SINGLE_PARAM sourceInformation

#ifdef _MSC_VER
#pragma detect_mismatch("WINRT_SOURCE_LOCATION", "true")
#endif
#else
#define WINRT_IMPL_SOURCE_LOCATION_ARGS_NO_DEFAULT
#define WINRT_IMPL_SOURCE_LOCATION_ARGS
Expand All @@ -96,5 +100,7 @@ typedef struct _GUID GUID;
#define WINRT_IMPL_SOURCE_LOCATION_FORWARD
#define WINRT_IMPL_SOURCE_LOCATION_FORWARD_SINGLE_PARAM

#ifdef _MSC_VER
#pragma detect_mismatch("WINRT_SOURCE_LOCATION", "false")
#endif
#endif
4 changes: 4 additions & 0 deletions strings/base_string_input.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@ WINRT_EXPORT namespace winrt::param
{
struct hstring
{
#ifdef _MSC_VER
#pragma warning(suppress: 26495)
#endif
hstring() noexcept : m_handle(nullptr) {}
hstring(hstring const& values) = delete;
hstring& operator=(hstring const& values) = delete;
hstring(std::nullptr_t) = delete;

#ifdef _MSC_VER
#pragma warning(suppress: 26495)
#endif
hstring(winrt::hstring const& value) noexcept : m_handle(get_abi(value))
{
}
Expand Down
2 changes: 1 addition & 1 deletion strings/base_version.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ extern "C"
__declspec(selectany)
char const * const WINRT_version = "C++/WinRT version:" CPPWINRT_VERSION;

#if defined(_MSC_VER)
#ifdef _M_IX86
#pragma comment(linker, "/include:_WINRT_version")
#else
#pragma comment(linker, "/include:WINRT_version")
#endif

#if defined(_MSC_VER)
#pragma detect_mismatch("C++/WinRT version", CPPWINRT_VERSION)
#endif

Expand Down
2 changes: 2 additions & 0 deletions test/old_tests/Component/pch.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#pragma once

#ifdef _MSC_VER
#pragma warning(disable:4100)
#endif

#include "winrt/Windows.Foundation.Collections.h"
#include "winrt/Composable.h"
Expand Down
2 changes: 2 additions & 0 deletions test/old_tests/Composable/precomp.hpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
#pragma once

#ifdef _MSC_VER
#pragma warning(disable:4100)
#endif

#include "winrt/Windows.Foundation.h"
#include "winrt/Composable.h"
2 changes: 2 additions & 0 deletions test/old_tests/UnitTests/Errors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ void test_exception(HRESULT const code, std::wstring_view message)
}
}

#ifdef _MSC_VER
#pragma warning(disable: 4702) // unreachable code
#endif
TEST_CASE("Errors")
{
// These won't throw.
Expand Down
4 changes: 3 additions & 1 deletion test/old_tests/UnitTests/IReference.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#include "pch.h"
#include "catch.hpp"

#ifdef _MSC_VER
#pragma warning(disable:4471) // a forward declaration of an unscoped enumeration must have an underlying type
#endif
#include <windows.foundation.h>

using namespace winrt;
Expand Down Expand Up @@ -55,4 +57,4 @@ TEST_CASE("IReference, set WinRT runtime class property")
{
HttpContentDispositionHeaderValue value(L"inline");
value.Size(200);
}
}
4 changes: 2 additions & 2 deletions test/old_tests/UnitTests/Main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ int main(int argc, char * argv[])
init_apartment();
std::set_terminate([]{ reportFatal("Abnormal termination"); ExitProcess(1); });
_CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
(void)_CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
_CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE);
_CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
(void)_CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
SetThreadUILanguage(MAKELANGID(LANG_ENGLISH, SUBLANG_ENGLISH_US));
int const result = Catch::Session().run(argc, argv);

Expand Down
11 changes: 11 additions & 0 deletions test/old_tests/UnitTests/apartment_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,21 @@ namespace
context2 = nullptr;
REQUIRE(!context2);

#ifdef __clang__
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wself-assign-overloaded"
#pragma clang diagnostic ignored "-Wself-move"
#endif
// Self-copy-assignment
context = context;
REQUIRE(context);

// Self-move-assignment
context = std::move(context);
REQUIRE(context);
#ifdef __clang__
#pragma clang diagnostic pop
#endif

co_await context;
}
Expand Down Expand Up @@ -76,13 +84,16 @@ namespace

#endif

// Exclude on mingw-w64 to suppress `-Wunused-function`
#if !defined(__MINGW32__)
bool is_nta_on_mta()
{
APTTYPE type;
APTTYPEQUALIFIER qualifier;
check_hresult(CoGetApartmentType(&type, &qualifier));
return (type == APTTYPE_NA) && (qualifier == APTTYPEQUALIFIER_NA_ON_MTA || qualifier == APTTYPEQUALIFIER_NA_ON_IMPLICIT_MTA);
}
#endif

bool is_mta()
{
Expand Down
7 changes: 7 additions & 0 deletions test/old_tests/UnitTests/com_ref.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,14 @@ namespace
}

#ifdef __CRT_UUID_DECL
#ifdef __GNUC__
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wunused-function"
#endif
__CRT_UUID_DECL(IClassic, 0x52bb7805, 0xe46e, 0x46f9, 0x85, 0x08, 0x86, 0x60, 0x6d, 0x2f, 0x6b, 0xc1);
#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif
#endif

TEST_CASE("com_ref agile_ref")
Expand Down
2 changes: 2 additions & 0 deletions test/old_tests/UnitTests/constexpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
#include "catch.hpp"
#include "string_view_compare.h"

#ifdef _MSC_VER
#pragma warning(disable:4471) // a forward declaration of an unscoped enumeration must have an underlying type
#endif
#include <Windows.Applicationmodel.Activation.h>

using namespace std::string_view_literals;
Expand Down
2 changes: 2 additions & 0 deletions test/old_tests/UnitTests/enum_flags.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#include "pch.h"
#include "catch.hpp"

#ifdef _MSC_VER
#pragma warning(disable:4471) // a forward declaration of an unscoped enumeration must have an underlying type
#endif
#include <Windows.ApplicationModel.Appointments.h>
#include "winrt/Windows.ApplicationModel.Appointments.h"

Expand Down
4 changes: 3 additions & 1 deletion test/old_tests/UnitTests/hresult_error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

// Missing in mingw-w64
#ifndef E_BOUNDS
#define E_BOUNDS (0x8000000B)
#define E_BOUNDS ((HRESULT)0x8000000B)
#endif

extern "C" BOOL __stdcall RoOriginateLanguageException(HRESULT error, void* message, void* languageException);
Expand Down Expand Up @@ -482,7 +482,9 @@ TEST_CASE("hresult, exception")
}
}

#ifdef _MSC_VER
#pragma warning(disable: 4702) // unreachable code
#endif
TEST_CASE("hresult, throw_last_error")
{
SetLastError(ERROR_CANCELLED);
Expand Down
2 changes: 2 additions & 0 deletions test/old_tests/UnitTests/meta.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
#include "pch.h"
#include "catch.hpp"

#ifdef _MSC_VER
#pragma warning(disable: 4505)
#endif

using namespace winrt;
using namespace Windows::Foundation;
Expand Down
3 changes: 2 additions & 1 deletion test/test/await_completed.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ namespace
// is the ABI breaking change with MSVC standard-conforming coroutines.)
REQUIRE(consumed <= sync_usage);
#else
(void)sync_usage;
// MSVC standard-conforming coroutines (as well as gcc and clang coroutines)
// support "bool await_suspend" just fine.
REQUIRE(consumed == 0);
Expand All @@ -71,4 +72,4 @@ namespace
TEST_CASE("await_completed_await")
{
SyncCompletion().get();
}
}
2 changes: 1 addition & 1 deletion test/test/custom_error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ TEST_CASE("custom_error_logger")
#endif

REQUIRE(s_loggerArgs.returnAddress);
REQUIRE(s_loggerArgs.result == 0x80000018); // E_ILLEGAL_DELEGATE_ASSIGNMENT)
REQUIRE(s_loggerArgs.result == static_cast<int32_t>(0x80000018)); // E_ILLEGAL_DELEGATE_ASSIGNMENT)

// Remove global handler
winrt_throw_hresult_handler = nullptr;
Expand Down
3 changes: 3 additions & 0 deletions test/test/disconnected.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,16 @@ namespace
co_return 123;
}

// Exclude on mingw-w64 to suppress `-Wunused-function`
#if !defined(__MINGW32__)
bool is_mta()
{
APTTYPE type;
APTTYPEQUALIFIER qualifier;
check_hresult(CoGetApartmentType(&type, &qualifier));
return type == APTTYPE_MTA;
}
#endif
}

TEST_CASE("disconnected,handler,1")
Expand Down
Loading