[clangd] Value initialize SymbolIDs

[kadircet]

Summary

We were default initializing SymbolIDs before, which would leave
indeterminate values in underlying std::array.

This patch updates the underlying data initalization to be value-init and adds a
way to check for validness of a SymbolID.

[sammccall]

clang-tools-extra/clangd/index/SymbolID.h

57

I'm not a big fan of LLVM's habit of calling the sentinel value "invalid".

Consider SourceLocation - one can construct invalid source locations that are isValid() (add an offset longer than the file) and SourceLocation() most commonly means "there is no location associated" rather than "we have bad data".

WDYT about calling this "isNull" (and possibly defining operator bool())?

[kadircet]

• change invalid -> null.
• add an implicit bool conversion operator.
• update apis returning optional<symbolid>s.

Note that all the functional changes are in SymbolID.h, rest is api updates.

[nridge]

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?

[sammccall]

In 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.

clang-tools-extra/clangd/AST.h

67 ↗

(On Diff #301678)

probably want to be a bit more specific about the possibility of returning null (and below)

clang-tools-extra/clangd/index/SymbolID.h

57

just return *this == SymbolID()?

58

nit: I think you want this to be explicit. Note that if(x) will perform an explicit cast if needed

58

this should be !isNull()!

[nridge]

In D90397#2366769, @sammccall wrote:

In 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.

Good point!

[kadircet]

• Mention possibility of returning null SymbolIDs in comments.
• Mark bool conversion operator explicit, fix the bug in returned value. (and run tests :))

clang-tools-extra/clangd/index/SymbolID.h

58

oopsy, should've ran tests :D

clang-tools-extra/clangd/index/SymbolID.h

 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_SYMBOLID_H
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 #include <array>
+#include <cstdint>
 #include <string>
 
 namespace clang {
 namespace clangd {
 
 // The class identifies a particular C++ symbol (class, function, method, etc).
 //
 // As USRs (Unified Symbol Resolution) could be large, especially for functions
public:
   constexpr static size_t RawSize = 8;
   llvm::StringRef raw() const;
   static SymbolID fromRaw(llvm::StringRef);
 
   // Returns a hex encoded string.
   std::string str() const;
   static llvm::Expected<SymbolID> fromStr(llvm::StringRef);
 
+  bool isValid() const { return HashValue != std::array<uint8_t, RawSize>{}; }
+
 private:
-  std::array<uint8_t, RawSize> HashValue;
+  std::array<uint8_t, RawSize> HashValue{};
 };
 
 llvm::hash_code hash_value(const SymbolID &ID);
 
 // Write SymbolID into the given stream. SymbolID is encoded as ID.str().
 llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const SymbolID &ID);
 
 } // namespace clangd