Skip to content

Use / for include paths, not \#18

Merged
kennykerr merged 1 commit intomicrosoft:masterfrom
LegalizeAdulthood:develop
Oct 20, 2016
Merged

Use / for include paths, not \#18
kennykerr merged 1 commit intomicrosoft:masterfrom
LegalizeAdulthood:develop

Conversation

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor

I was an MVP for Direct3D for 10 years, you might already have a CLA on file for me...

@raffaeler
Copy link
Copy Markdown

I believe it does not make sense to make pull requests on the libraries as it is all generated code.
The patch should be always applied to the generator.

@BrentRe
Copy link
Copy Markdown
Contributor

BrentRe commented Oct 19, 2016

Actually this particular request is to update the Sample projects, which are not generated code.

@raffaeler
Copy link
Copy Markdown

oops, sorry

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor Author

Yes, as @BrentRe pointed out, the libraries are fine, it's just the sample code that is using \ instead of /.

They also use #pragma lib, which isn't portable C++, but noone else besides Windows has a WinRT runtime anyway, so I doubt that part really matters.

@BrentRe
Copy link
Copy Markdown
Contributor

BrentRe commented Oct 19, 2016

We could move the #pragma lib into the VC++ project settings to remove it. But as all these samples are Windows-specific, I'm not sure there's much benefit. The reasoning for putting in the source code was to alert developers that Windows Store applications must link with that library. Something that's easily missed when it buried in a linker command line setting.

For the record, what are you suggesting to change from back slashes to forward slashes? I'm guessing because both work and forward is more platform agnostic... I'm not pushing back but these samples are very Windows specific (as is C++/WinRT currently).

@LegalizeAdulthood
Copy link
Copy Markdown
Contributor Author

Using \ in the paths of include files only works on Windows and inculcates bad habits in C++ developers, so yes that is what this patch fixes.

@kennykerr kennykerr merged commit a890b90 into microsoft:master Oct 20, 2016
@kennykerr
Copy link
Copy Markdown
Collaborator

Thanks for the contribution Richard!

@LegalizeAdulthood LegalizeAdulthood deleted the develop branch October 20, 2016 22:56
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