[clangd] NFC: Use uint32_t for FuzzyFindRequest limits

[ioeric]

clang-tools-extra/clangd/index/Index.cpp

186 ↗

(On Diff #164680)

I think we should not set Request.MaxCandidateCount if MaxCandidateCount is greater than std::numeric_limits<int32_t>::max()?

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

440 ↗

(On Diff #164680)

Or use unsigned?

[sammccall]

Why this change?

(If you're changing it, Optional<unsigned> is probably clearer)

[kbobyrev]

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

440 ↗

(On Diff #164680)

unsigned would have different size on different platforms, I'm not really sure we want that; could you elaborate on why you think that would be better?

[ioeric]

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

440 ↗

(On Diff #164680)

I thought it's (almost) always 4 bytes? But it should always have a smaller size than uint64_t in json serialization, so it should work for us. In general, I would prefer unsigned to uint32_t when possible. For most of the platforms, they are the same. But up to you :) I don't really feel strong about this.

[ilya-biryukov]

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

440 ↗

(On Diff #164680)

BTW, many people think using unsigned ints just because inputs can't be negative is a bad idea.
See https://stackoverflow.com/a/18796234

[sammccall]

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

440 ↗

(On Diff #164680)

I mostly agree with that, but LLVM uses unsigned types pervasively, and -Wsign-compare, so they're hard to avoid.

(FWIW, I still think that this case has become complicated enough that we should use the most explicit option, which seems like Optional here)

[ioeric]

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

440 ↗

(On Diff #164680)

The complication seems to be mostly in json serialization of the field, which is more of an implementation detail I think. If we could get away with using max unsigned as no limit, which seems to have worked well so far, I would still prefer it over Optional, as it is less confusing on the reference site and involves less refactoring.

[ilya-biryukov]

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

440 ↗

(On Diff #164680)

+1 to using Optional, this states the contract better and actually gives a clear model of how to should be serialized and deserialized.

Can see the logic in using large values for the limits too, but optional seems to avoid various corner cases, which outweighs the refactoring costs IMO.

Feel free to ignore, though, just a drive-by-comment.

[kbobyrev]

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

440 ↗

(On Diff #164680)

Yes, AFAIK Google Coding Guidelines prohibit usage of unsigned (e.g. in for loops), but I don't know whether I feel good about that.

As for the llvm::Optional, I can understand arguments for and against it, but it might be OK to use the proposed approach for now as it doesn't require any refactoring and add optional later if I (or someone else) has time to make sure the user code doesn't make anything unexpected. I'll put a FIXME on that, will probably fix later if I'll get enough time. Also, it might make sense to rename it to Limit or something similar.

clang-tools-extra/trunk/clangd/index/Index.h

struct FuzzyFindRequest {
   /// (e.g. namespaces) excluding nested scopes. For example, if a scope "xyz::"
   /// is provided, the matched symbols must be defined in namespace xyz but not
   /// namespace xyz::abc.
   ///
   /// The global scope is "", a top level scope is "foo::", etc.
   std::vector<std::string> Scopes;
   /// \brief The number of top candidates to return. The index may choose to
   /// return more than this, e.g. if it doesn't know which candidates are best.
-  size_t MaxCandidateCount = std::numeric_limits<size_t>::max();
+  // FIXME: Use llvm::Optional; semantically, the absence of MaxCandidateCount
+  // is equivalent to setting this field to default value as below.
+  uint32_t MaxCandidateCount = std::numeric_limits<uint32_t>::max();
   /// If set to true, only symbols for completion support will be considered.
   bool RestrictForCodeCompletion = false;
   /// Contextually relevant files (e.g. the file we're code-completing in).
   /// Paths should be absolute.
   std::vector<std::string> ProximityPaths;
 
   bool operator==(const FuzzyFindRequest &Req) const {
     return std::tie(Query, Scopes, MaxCandidateCount, RestrictForCodeCompletion,

clang-tools-extra/trunk/clangd/index/Index.cpp

 }
 std::shared_ptr<SymbolIndex> SwapIndex::snapshot() const {
   std::lock_guard<std::mutex> Lock(Mutex);
   return Index;
 }
 
 bool fromJSON(const llvm::json::Value &Parameters, FuzzyFindRequest &Request) {
   json::ObjectMapper O(Parameters);
-  llvm::Optional<int64_t> MaxCandidateCount;
+  int64_t MaxCandidateCount;
   bool OK =
       O && O.map("Query", Request.Query) && O.map("Scopes", Request.Scopes) &&
+      O.map("MaxCandidateCount", MaxCandidateCount) &&
       O.map("RestrictForCodeCompletion", Request.RestrictForCodeCompletion) &&
-      O.map("ProximityPaths", Request.ProximityPaths) &&
-      O.map("MaxCandidateCount", MaxCandidateCount);
-  if (MaxCandidateCount)
-    Request.MaxCandidateCount = MaxCandidateCount.getValue();
+      O.map("ProximityPaths", Request.ProximityPaths);
+  if (OK && MaxCandidateCount <= std::numeric_limits<uint32_t>::max())
+    Request.MaxCandidateCount = MaxCandidateCount;
   return OK;
 }
 
 llvm::json::Value toJSON(const FuzzyFindRequest &Request) {
-  auto Result = json::Object{
+  return json::Object{
       {"Query", Request.Query},
       {"Scopes", json::Array{Request.Scopes}},
+      {"MaxCandidateCount", Request.MaxCandidateCount},
       {"RestrictForCodeCompletion", Request.RestrictForCodeCompletion},
       {"ProximityPaths", json::Array{Request.ProximityPaths}},
   };
-  // A huge limit means no limit, leave it out.
-  if (Request.MaxCandidateCount <= std::numeric_limits<int64_t>::max())
-    Result["MaxCandidateCount"] =
-        static_cast<int64_t>(Request.MaxCandidateCount);
-  return std::move(Result);
 }
 
 bool SwapIndex::fuzzyFind(const FuzzyFindRequest &R,
                           llvm::function_ref<void(const Symbol &)> CB) const {
   return snapshot()->fuzzyFind(R, CB);
 }
 void SwapIndex::lookup(const LookupRequest &R,
                        llvm::function_ref<void(const Symbol &)> CB) const {