kbobyrev added a comment.

Thank you for taking care of this!



================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:261
+    if (!FE) {
+      assert(isSpecialBuffer(FID, SM));
+      continue;
----------------
Implementation-wise, this seems like the right place to drop nonexistent 
"headers". However, this is a very surprising behavior from something like 
"translateToHeaderIDs" for two reasons:

- As the name states, it used to be a simple "translation" process, now it's 
also filtering.
- `TranslatedHeaderIDs.size() == Files.size()` can be rather unexpected

We never had 1:1 order etc translation assumptions but this is certainly the 
new behavior we want to document.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:261
+    if (!FE) {
+      assert(isSpecialBuffer(FID, SM));
+      continue;
----------------
kbobyrev wrote:
> Implementation-wise, this seems like the right place to drop nonexistent 
> "headers". However, this is a very surprising behavior from something like 
> "translateToHeaderIDs" for two reasons:
> 
> - As the name states, it used to be a simple "translation" process, now it's 
> also filtering.
> - `TranslatedHeaderIDs.size() == Files.size()` can be rather unexpected
> 
> We never had 1:1 order etc translation assumptions but this is certainly the 
> new behavior we want to document.
Thinking out loud: now I'm also thinking about the "handle non self-contained 
files" + "drop warnings for files without include warnings"


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:275
+        SM.getPresumedLoc(SM.getLocForStartOfFile(FID)).getFilename());
+  // (Note we deduped the names as _number_ of <built-in>s is uninteresting).
+  EXPECT_THAT(ReferencedFileNames.keys(),
----------------
nit: Drop the parenthesis?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112652

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to