Page MenuHomePhabricator

Auto-install Clang Visual Studio visualizers for VS2015 and up
ClosedPublic

Authored by mspertus on Mar 27 2016, 1:28 AM.

Details

Summary

This change uses the Project Support for Natvis added in VS2015 to eliminate the need to manually install natvis files. D18497 is a corresponding diff for LLVM. I want to acknowledge ariccio for extensive advice on this change.

Diff Detail

Event Timeline

mspertus updated this revision to Diff 51731.Mar 27 2016, 1:28 AM
mspertus retitled this revision from to Auto-install Clang Visual Studio visualizers for VS2015 and up.
mspertus updated this object.
mspertus added a subscriber: cfe-commits.
mspertus updated this revision to Diff 51741.Mar 27 2016, 9:09 AM

Apply whitespace comments from D18497 mutatis mutandis to this change

zturner edited edge metadata.Mar 27 2016, 9:22 AM
zturner added a subscriber: zturner.

For vs2015, the files still need to be in the project right? (In the
vcxproj with a <natvis> tag)

For vs2015, the files still need to be in the project right? (In the
vcxproj with a <natvis> tag)

This change puts the natvis files in the project for VS2015 but it appears to use the none tag

<?xml version="1.0" encoding="UTF-8"?>
<Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup>
    <CustomBuild Include="D:\t2\llvm\tools\clang\CMakeLists.txt" />
  </ItemGroup>
  <ItemGroup>
    <None Include="D:\t2\llvm\tools\clang\utils\clang.natvis" />
  </ItemGroup>
  <ItemGroup>
  </ItemGroup>
</Project>

It does seem to work correctly though... Whaddaya think?
mspertus added a comment.EditedMar 27 2016, 10:04 AM

Hmmm, I just tried adding set_source_files_properties(utils/clang.natvis LANGUAGE natvis) similar to what you suggest in http://lists.llvm.org/pipermail/llvm-dev/2016-January/093887.html, but it still produces the None tag in the .vcxproj file.

Edit: Reading the link more carefully, it looks like CMake would need to be modified to support Natvis as a language, which seems out-of-scope for Clang (plus, forcing people to get a new version of CMake just for this might be overkill). I think leaving the change the way it is is probably best as it doesn't cause VS2015 any trouble. Is that ok?

Yea, CMake doesn't actually support natvis files. You'd have to actually
patch CMake to make it work.

Can you make the vs2015 and vs2013 paths the same? It will still work for
2015 i think, just that 2015 also supports putting them in the project file

What do you mean by "making both paths the same"?

Code paths. For vs2013 you're installing the natvis files, can we just do
that for vs2015 as well?

mspertus added a comment.EditedMar 27 2016, 11:54 AM

I'm not sure I understand. I'm not installing anything for VS2013. You still have to manually copy into Documents\Visual Studio 2013\Visualizers. The purpose of this change is to eliminate this manual step for VS2015 by leveraging the new Natvis project support. Am I missing something?

If it's not using a natvis tag in the vcxproj (which it isn't since CMake
doesn't support it during generation) then the new natvis project file
support won't work right? In which case the files would need to be copied
into documents\vs2015, otherwise it won't work, unless I'm misunderstanding
something

No. Testing shows that it works fine in the project with the <None> tag. VS2015 empirically looks at the extension.

Ahh that's surprising. If it works even with the none tag, i guess my
concerns are not valid

I understand your concerns, but on balance, I don't see a better course of action than what I've done. If I have some time, I'll submit a CMake change so we can eventually migrate to having CMake generate a Natvis tag, but that will take a long time to propagate, so I don't want to delay this operationally semantically equivalent change. Sound ok?

Yea sounds fine

mspertus updated this revision to Diff 51759.Mar 27 2016, 4:57 PM
mspertus edited edge metadata.

Now that we are generating projects for native visualizers, have it properly reflected in the directory structure

ariccio edited edge metadata.Mar 27 2016, 5:29 PM

Ahh that's surprising. If it works even with the none tag, i guess my
concerns are not valid

I'm only partly surprised... It's not the first time that a Microsoft product behaves surprisingly :)

Assuming everything builds correctly, LGTM.

Your CMake is better than mine, so I'm not sure if there're better ways to do this ;)

utils/ClangVisualizers/CMakeLists.txt
3

Obsessive nit: Use a period after the "want", like want.

utils/ClangVisualizers/clang.natvis
9

Is it just me, or is this a dead link?

mspertus marked an inline comment as done.Mar 27 2016, 6:13 PM
mspertus added inline comments.
utils/ClangVisualizers/CMakeLists.txt
3

Will do. Note that unfortunately, the CMakeLists.txt files are already inconsistent on that score...

utils/ClangVisualizers/clang.natvis
9

It's not a link, it's an XML namespace. The idea is that you use a URL that you control as a namespace identifier to avoid collisions.

mspertus marked an inline comment as done.Mar 27 2016, 6:14 PM
mspertus updated this revision to Diff 51762.Mar 27 2016, 6:17 PM
mspertus edited edge metadata.

Incorporating ariccio's comments and fixing some html

ariccio added inline comments.Mar 28 2016, 12:28 AM
utils/ClangVisualizers/CMakeLists.txt
3

Hmm, that's gonna bug me. Maybe I'll just have to proofread all of them.

utils/ClangVisualizers/clang.natvis
9

Ahh, that's actually quite smart.

This is still showing as "Needs review" Are there any further changes required or can someone accept it? I installed both VS2015 and VS2013 on my laptop and saw the correct solution files built by cmake.

zturner accepted this revision.Mar 28 2016, 10:55 AM
zturner edited edge metadata.
This revision is now accepted and ready to land.Mar 28 2016, 10:55 AM

Commited as revision 264603

mspertus closed this revision.Mar 28 2016, 11:09 AM

Closing diff after commit

I installed both VS2015 and VS2013 on my laptop and saw the correct solution files built by cmake.

Btw, make sure you're running the latest updates for both Visual Studio, else you'll get funny build errors (probably about /Zc:strictStrings).