[PATCH] D65754: Fix toHalfOpenFileRange assertion fail

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:307
+// the same file id.
+static SourceRange getExpansionRangeInSameFiles(SourceLocation Loc,
+                                                const SourceManager &SM,
----------------
nit: `getExpansionTokenRangeInSameFile`?


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:315
+    return ExpansionRange;
+  // Expand begin of Expansion range till the top and map with file id hash.
+  llvm::DenseMap<unsigned, SourceRange> BeginExpansions;
----------------
Having a little trouble parsing this comment...

"Record the stack of expansion locations for the beginning, keyed by FileID"?


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:316
+  // Expand begin of Expansion range till the top and map with file id hash.
+  llvm::DenseMap<unsigned, SourceRange> BeginExpansions;
+  SourceRange BeginExpansion = ExpansionRange.getBegin();
----------------
DenseMap<FileID, SourceRange>

Why are the values SourceRange here and not just SourceLocation?


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:318
+  SourceRange BeginExpansion = ExpansionRange.getBegin();
+  while (1) {
+    BeginExpansions[SM.getFileID(BeginExpansion.getBegin()).getHashValue()] =
----------------
Is there a way to write this loop that better captures the intent? while(1) with a break in the middle is hard for readers to understand at a glance.

I think it's just

```
for (SourceLocation Begin = ExpansionRange.getBegin();
       Begin.isValid();
       Begin = Begin.isFileId() ? SourceLocation() : SM.getImmediateExpansionRange(Begin).getBegin())
  BeginExpansions[SM.getFileID(Begin)] = Begin;
```


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:323
+      break;
+    BeginExpansion = toTokenRange(
+        SM.getImmediateExpansionRange(BeginExpansion.getBegin()), SM, LangOpts);
----------------
this call to toTokenRange seems unneccesary, that just adjusts End, but we only care about Start


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:328
+  // BeginExpansions.
+  SourceRange EndExpansion = ExpansionRange.getEnd();
+  while (1) {
----------------
again, I think by treating these as ranges rather than points you're making it harder than it needs to be. Traversing up the expansion tree isn't going to invert the order (though spelling might)


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:333
+    if (It != BeginExpansions.end())
+      return unionTokenRange(It->second, EndExpansion, SM, LangOpts);
+    if (EndExpansion.getEnd().isFileID())
----------------
isn't this always SourceRange(It->second.getBegin(), EndExpansion.getEnd())?


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:362
+      assert(inSameFile(FileRange.getBegin(), FileRange.getEnd(), SM) &&
+             "Both ends should be in same file.");
     } else {
----------------
nit: as with comments, avoid adding messages to the assertion unless it adds something to the code


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65754