Closed sylveon closed 1 year ago
This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.
Bump
Unfortunately, a project maintainer is not currently available to review this pull request. Please see the contributing guide for more information. Feel free to keep the conversation going on the related issue.
Per discussion thread, reopening
@jonwis - this change is going to be harder to review as it changes code gen which can't be directly reviewed just by looking at the PR even with good test coverage. I tend to take such PRs, clone the branch, and manually verify the code gen looks sound. Its tedious but the repercussions from bad code gen can be expensive if detected later on.
p.s. for windows-rs I opted to include code gen in the repo for test verification and diffing which helps to automate this.
I thought I had forgot about protected constructors, but it turns out protected constructors are already hidden from code which doesn't compose. I added test coverage for it as I couldn't find any.
This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.
I'll take this on Monday to compare the generated headers and some sample compiled code. I like the idea that we add some kind of "diff output" script that uses the current release version and the just-built version then your favorite directory-diffing tool to see what changed.
The source-breaking aspect has me concerned given how large the Windows OS codebase is and how widespread protected members are in WinUI3.
I know we don't like behavioral modification switches that change codegen, but this definitely is a place where it might be better to allow a "enable protected member access by namespace filter" control out.
how widespread protected members are in WinUI3.
Most WinUI 3 code is in C#, where protected members are hidden. I'd expect the amount of C++ developers which are actively using protected members against guidance to be fairly low.
enable protected member access by namespace filter
There is no precedent for per-namespace alterations of this kind. I would suggest you start without an escape hatch and see how bad the breaks are in practice by performing an OS build. You may find that they are easily manageable. Once you have a good idea of what the practical implications are, add an escape hatch as needed and only if absolutely necessary. The approach C++/WinRT has used in the past is to use a preprocessor definition like WINRT_LEAN_AND_MEAN
. This will be a lot simpler to implement and manage.
https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/macros
This is how I upgraded the OS from C++/WinRT 1.0 to 2.0. But again, you need to get the OS repo up to date first so that you can easily tell the delta of this change specifically. Otherwise, you will just be fighting a slew of random breaks.
Yeah, and namespace filtering is non-trivial to implement.
OK, https://github.com/sylveon/cppwinrt/pull/1 adds new differencing tools to compare the latest release and the under-development codegen. The changes here seem expected - mostly limited to XAML types, for instance. Will need to ingest into the OS carefully.
Unfortunately this does seem to be a breaking change, at least for some projects :(. I'm trying to build our code base with a private cppwinrt.exe that I built from the HEAD of master and I have some build breaks that seem to match this signature. (The project is currently on the most recent published release 2.0.230706.1 which narrowly missed this change).
Are those breaking changes perhaps caused by said projects using protected/overridable members? :P
Probably. They are using something from Xaml::Controls::IControlProtected
. It even has Protected in the name 🤷.
Unfortunately this usage is a pattern established in an (internal) project template so there are dozens of components that will have this same break to fix. At least this particular fix is not especially difficult.
According to the WinRT type system:
Previously, cppwinrt allowed anyone to directly call protected/overridable members, for example this code works
This change makes it so that only implementation types which derive from the base have access to protected and overridable members.
This change is potentially breaking, but code that this change breaks should've never compiled to begin with (and indeed the equivalents in C++/CX as well as .NET Framework and .NET Core do not build).
In worse case, if access to protected and overridable members is desired (despite this being against the WinRT type system), one can do
base.as<IBaseProtected>().ProtectedMember()
There's a potential gain to compile-time when
Windows.UI.Xaml
is involved, since a bunch of consume_t classes for overridable/protected members no longer get instantiated.Fixes: #1317