Skip to content

Fix include path for generated component (.g.cpp) files #537

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

Merged
kennykerr merged 2 commits into from
Feb 21, 2020
Merged

Fix include path for generated component (.g.cpp) files #537

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d405e61
Fix include path for generated component (.g.cpp files)
Feb 21, 2020
5d3f655
Merge branch 'master' into fix-g-cpp-include
kennykerr Feb 21, 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
6 changes: 4 additions & 2 deletions cppwinrt/component_writers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1152,9 +1152,9 @@ namespace winrt::@::implementation

static void write_component_cpp(writer& w, TypeDef const& type)
{
auto filename = get_component_filename(type);

{
auto filename = get_component_filename(type);

auto format = R"(#include "%.h"
)";

Expand All @@ -1163,6 +1163,8 @@ namespace winrt::@::implementation

if (settings.component_opt)
{
auto filename = get_generated_component_filename(type);
Copy link
Collaborator

@kennykerr kennykerr Feb 21, 2020

Choose a reason for hiding this comment

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

Have you tested with or without -prefix? Any change like would need to be justified in detail and then tested very heavily as there are a lot of projects publicly and inside the Windows OS that rely on historical quirky behavior related to include paths.

Copy link
Contributor Author

@mlalic mlalic Feb 21, 2020

Choose a reason for hiding this comment

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

Yes, I have tested with and without -prefix. This change works correctly in both cases. (When -prefix is specified, it still emits the include in the form of Foo.Bar.Baz.g.cpp, as expected and as before the change.) The reason it works is because get_generated_component_filename conditionally generates the directory path or the dotted prefix file based on whether the option is provided or not.

I think the description is already pretty detailed, to be honest, but I am happy to add more information that you feel might be missing.

The crux of the issue here isn't really the include path, but simply the file name that is emitted in the include directive is never created (if -prefix isn't specified and -name is unable to fully strip the namespace prefix). The actual file name ends up being Baz.g.cpp, but we're emitting an include for Foo.Bar.Baz.g.cpp. That's clearly not going to work regardless of include path configuration. :) And I discovered this in the OS repo itself, where I had to work around it by manually modifying the emitted include path :)

Further, this is a change to what's emitted to a file that is already meant to be edited manually (we now even have a static_assert to that effect) so risk of breaking any existing project that uses cppwinrt is extremely low.

Finally, as I wrote in the description, this uses the exact same function to generate the included file name as is used to emit the include for the .g.h file in the component's .h file. .g.cpp files are generated using the same code as .g.h files and wind up in the same location and therefore should use the same approach to emit the include.

Copy link
Collaborator

@kennykerr kennykerr Feb 21, 2020

Choose a reason for hiding this comment

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

Excellent, thanks for the due diligence. Obviously I'm all for fixing broken scenarios. Just cautious about breaking existing ones. We just don't have sufficient test collatoral to catch breaks early and its a lot more painful to catch them when a branch build breaks (as I'm sure you're aware).

Copy link
Contributor Author

@mlalic mlalic Feb 21, 2020

Choose a reason for hiding this comment

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

Understood, of course. :)


auto format = R"(#include "%.g.cpp"
)";

Expand Down