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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
