[PATCH] D89882: [clangd] Offset enum values by when marshalling

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/index/remote/Index.proto:15
 // structures corresponding to their clangd::* counterparts.
+// NOTE: Enum values are offset by one to detect missing values.
 service SymbolIndex {
----------------
can we just switch to proto2 instead?

apart from reserving the zero value, proto3 also requires you to explicitly handle zero in your code (vs being able to set a default in the proto definition)

Offsetting enums by one isn't a big deal, but it can be in other cases (consider a number like num_refs where 0 is a meaningful value and offsetting by 1 is really confusing)


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:115
   Req.IDs = std::move(*IDs);
-  Req.Filter = static_cast<RefKind>(Message->filter());
+  Req.Filter = static_cast<RefKind>(Message->filter() - 1);
   if (Message->limit())
----------------
this -1 and +1 everywhere makes me a bit leery.
Seems a bit easy to reverse or forget it.

What about

```
template <typename T> std::underlying_type_t<T> toProtobuf(T Value) {
  return static_cast<std::underlying_type_t<T>>(Value) + 1;
}
```

and the inverse?


================
Comment at: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp:277
   }
-  Result.set_flags(static_cast<uint32_t>(From.Flags));
+  Result.set_flags(static_cast<uint32_t>(From.Flags) + 1);
   return Result;
----------------
?!

this is a bitfield


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89882