Skip to content

Conversation

@sylveon
Copy link
Contributor

@sylveon sylveon commented Sep 26, 2022

This makes guid_v (and by extension name_v) constexpr on Clang, improves the error reporting when unknwn.h is not included before winrt/base.h (gives a compile-time error instead of an empty GUID and potential ODR violations), and finally brings back the ability to use classic COM without including unknwn.h before winrt/base.h on MSVC.

Fixes #1186

@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kennykerr
Copy link
Collaborator

@DefaultRyan know why the build's failing? Is the visualizer not building cleanly?

@sylveon
Copy link
Contributor Author

sylveon commented Sep 26, 2022

I noticed the visualizer doesn't build with clang, but since it appeared that clang was already broken in a few other ways (e.g. unit tests don't build without enabling C++20 and disabling the PCH) I didn't expect that to be tested. If that's the issue I can take a look at fixing it.

@kennykerr
Copy link
Collaborator

Yes, the build seems to be unhappy with at least some of the warnings. Thanks for taking a look.

oldnewthing
oldnewthing previously approved these changes Sep 26, 2022
sylveon and others added 3 commits September 27, 2022 01:15
Co-authored-by: Raymond Chen <oldnewthing@users.noreply.github.com>
@sylveon
Copy link
Contributor Author

sylveon commented Sep 27, 2022

Clang fails building the Natvis visualizer due to code in the Microsoft.VSSDK.Debugger.VSDebugEng NuGet package. Specifically, it's the following pattern, that attempts to emulate enum class:

class foo
{
public:
    struct bar { enum e; };

    enum bar::e
    {
        buz
    };
};

Forward declaring enums is usually not allowed by the standard but Clang will let it fly with -fms-compatibility. However, it doesn't handle the case where you then try define an enum nested in a struct nested in a class (because normally only friends in a class can have a qualified name).

Thankfully, it seems like there is a workaround: defining VSDEBUGENG_USE_CPP11_SCOPED_ENUMS makes the header use enum class. I've enabled it unconditionally.

Another issue that showed up is that the PCH for the visualizer includes base_includes.h which includes <experimental/coroutine> (which doesn't works under Clang) if C++20 coroutines aren't available, so I've upgraded the visualizer project to C++20, which solves the issue by making it include standard <coroutine> (which works under Clang). Coroutines don't appear used in that project so this shouldn't result in any issue.

This allows the visualizer to build with Clang. The visualizer not building is what I guess was the issue, no idea if it actually is. If it isn't, then I would probably need more details to be able to fix it.

@kennykerr
Copy link
Collaborator

azp /run

@sylveon
Copy link
Contributor Author

sylveon commented Sep 27, 2022

I believe you want /azp run 😉

@kennykerr
Copy link
Collaborator

kennykerr commented Sep 27, 2022

I believe you want /azp run 😉

Indeed. 🤣

I typed that on my phone. Not a good idea...

@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kennykerr
Copy link
Collaborator

woohoo build is clean - thanks @sylveon!

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.

Thanks!

@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kennykerr kennykerr merged commit e7b6903 into microsoft:master Sep 29, 2022
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.

Zero GUID when unknwn.h is not included

4 participants