[PATCH] D128457: [clangd] Add new IncludeType to IncludeHeaderWithReferences

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/Symbol.h:93
+    Import = 2,
+  };
+
----------------
you can use `LLVM_MARK_AS_BITMASK_ENUM` and relevant magic to implement bitwise operators on the type (see llvm-project/llvm/include/llvm/ADT/BitmaskEnum.h)


================
Comment at: clang-tools-extra/clangd/index/Symbol.h:116
+    /// this header.
+    IncludeTypes IncludeTypes = Invalid;
   };
----------------
the naming here feels a little confusing, can we change the enum name to be `IncludeDirective` and field name to `SupportedDirectives`


================
Comment at: clang-tools-extra/clangd/index/Symbol.h:112
     /// this header. This number is only meaningful if aggregated in an index.
     unsigned References = 0;
+    /// Type of include to use when including this header, #include or #import.
----------------
i think we already rely on unsigned being 32 bits here. instead of introducing a new byte, can we make `References` 30 bits long and use 2 bits for the include type? also change the type from unsigned to `uint32_t`.


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:110
   optional uint32 references = 2;
+  optional uint32 include_types = 3;  // Actually uint8_t.
 }
----------------
no need for the comment here.


================
Comment at: clang-tools-extra/clangd/unittests/SerializationTests.cpp:59
     References:    3
+    Type:      [ Import ]
 ...
----------------
can we also have an example for both (and none)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128457/new/

https://reviews.llvm.org/D128457