Skip to content

Conversation

@jlaanstra
Copy link
Contributor

@jlaanstra jlaanstra commented May 20, 2021

Fixes #945. Given that the component headers already use this I don't think this would break C++/WinRT compatibility with compilers that don't support this (technically it's non-standard).

Question is if we need to keep the open file guard?

@jlaanstra jlaanstra requested a review from kennykerr May 20, 2021 04:06
@kennykerr
Copy link
Collaborator

I vaguely recall having to use ifdef guards to avoid compatibility problems and using pragma once caused other problems for some compilers, but perhaps those issues have since been dealt with by the compilers.

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

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

I'm happy to accept this change, but its possible this will break in some obscure way in the OS build. If that happens, its simple enough to revert.

@jlaanstra
Copy link
Contributor Author

Let me send mail to see whatI can find out, before merging.

@kennykerr
Copy link
Collaborator

#945 (comment)

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

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

It seems like we don't have a compelling reason to take this change. There's no data that this improves performance and some risk that this breaks the OS build.

@jlaanstra
Copy link
Contributor Author

There is at least the argument of consistency that makes it worth it, even if there is no large perf benefit? Especially given that the stl follows the same pattern?

@kennykerr
Copy link
Collaborator

I don't mind the change itself - I'm just concerned about the fallout from the OS build - ChrisG's comment reminded me that I'd seen that very issue in the OS build before when using pragma once with C++/WinRT. I don't know if it still exists but it would burn a Windows build finding out. I'm happy if you want to do that finding out though. 😉

@jlaanstra
Copy link
Contributor Author

Let's find out :). Worst case I learn something new.

@kennykerr kennykerr merged commit f204e39 into master May 20, 2021
@kennykerr kennykerr deleted the user/jlaans/945 branch May 20, 2021 20:38
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.

Generated headers should use #pragma once in addition to header guard

3 participants