[PATCH] D114949: [clangd] IncludeCleaner: Do not require forward declarations of RecordDecls when definition is available

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:151
+      if (SM.isInMainFile(RD->getMostRecentDecl()->getLocation())) {
+        if (Definition)
           Result.insert(Definition->getLocation());
----------------
if we made it here, we already know definition wasn't visible in the current TU.

maybe just something like:

```
// Special case RecordDecls, as it is common for them to be forward declared multiple times.
// The most common two cases are:
// - definition available in TU, only mark that one as usage. rest is likely to be unnecessary. this might result in false positives when an internal definition is visible.
// - there's a forward declaration in the main file, no need for other redecls.
if (auto *RD = ...) {
  if (auto *Def) {
    insert that;
    return;
  }
  if (mostRecentInMainFile) {
    return;
  }
}
```

If we really want to keep a note around nested decls, we might say something like:
`Forward decls/definitions of nested recorddecls can only be nested inside other recorddecls, and the definition for outer recorddecl has to be visible, and we always preserve definitions.`

But I don't think all the additional information provides much value, rather creates confusion and forces one to think more about the details.


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:67
       {
-          "namespace ns { struct ^X; struct ^X {}; }",
+          "namespace ns { struct X; struct ^X {}; }",
           "using ns::X;",
----------------
you mind changing these into functions as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114949