[PATCH] D90397: [clangd] Value initialize SymbolIDs

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

In D90397#2366727 <https://reviews.llvm.org/D90397#2366727>, @nridge wrote:

> Would it have been possible to disallow default-constructing `SymbolID` altogether, and preserve the ability to represent "an always-valid symbol id" (`SymbolID`) vs. "a maybe-valid symbol id" (`Optional<SymbolID>`) as distinct types in the type system?

Absolutely, except where it matters`sizeof(SymbolID)==8` and `sizeof(Optional<SymbolID>)==16`.
I think the trigger here was `SymbolID Ref::Container` - we can't afford to use `Optional` there.
We could come up with some special-case solution for that and use Optional elsewhere - I don't really feel strongly about it.



================
Comment at: clang-tools-extra/clangd/AST.h:67
 
 /// Gets the symbol ID for a declaration, if possible.
+SymbolID getSymbolID(const Decl *D);
----------------
probably want to be a bit more specific about the possibility of returning null (and below)


================
Comment at: clang-tools-extra/clangd/index/SymbolID.h:57
 
+  bool isNull() const { return HashValue != std::array<uint8_t, RawSize>{}; }
+  operator bool() const { return isNull(); }
----------------
just `return *this == SymbolID()`?


================
Comment at: clang-tools-extra/clangd/index/SymbolID.h:58
+  bool isNull() const { return HashValue != std::array<uint8_t, RawSize>{}; }
+  operator bool() const { return isNull(); }
+
----------------
nit: I think you want this to be explicit. Note that if(x) **will** perform an explicit cast if needed


================
Comment at: clang-tools-extra/clangd/index/SymbolID.h:58
+  bool isNull() const { return HashValue != std::array<uint8_t, RawSize>{}; }
+  operator bool() const { return isNull(); }
+
----------------
sammccall wrote:
> nit: I think you want this to be explicit. Note that if(x) **will** perform an explicit cast if needed
this should be `!isNull()`!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90397