[PATCH] D48703: Add natvis files directly to the PDB instead of to the VS solution

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: clang/CMakeLists.txt:345
 
+if (LINKER_IS_LLD_LINK OR LINKER_IS_MSVC_LINK)
+  set(CMAKE_EXE_LINKER_FLAGS
----------------
I'm wondering if it would be nicer to do a check_linker_flag_exists for `/natvis`. I'm fine with this though.


================
Comment at: clang/CMakeLists.txt:348
+    "${CMAKE_EXE_LINKER_FLAGS} /natvis:${CMAKE_CURRENT_SOURCE_DIR}/utils/ClangVisualizers/clang.natvis")
+  set(CMAKE_SHARED_LINKER_FLAGS
+    "${CMAKE_SHARED_LINKER_FLAGS} /natvis:${CMAKE_CURRENT_SOURCE_DIR}/utils/ClangVisualizers/clang.natvis")
----------------
I don't think it makes too much of a difference right now, but for completeness, it would be nice to set CMAKE_MODULE_LINKER_FLAGS as well.


================
Comment at: llvm/utils/LLVMVisualizers/llvm.natvis:198
   <Type Name="llvm::Optional<*>">
-    <DisplayString Condition="!hasVal">empty</DisplayString>
-    <DisplayString Condition="hasVal">{*(($T1 *)(unsigned char *)storage.buffer)}</DisplayString>
+    <DisplayString Condition="!Storage.hasVal">None</DisplayString>
+    <DisplayString Condition="Storage.hasVal">{*(($T1 *)(unsigned char *)Storage.storage.buffer)}</DisplayString>
----------------
I think it would be better to split the changes to this file out into its own diff (perhaps even two diffs, one for the fix and one for the new visualizer).


https://reviews.llvm.org/D48703