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

Kadir Cetinkaya via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Dec 2 06:33:23 PST 2021

kadircet added inline comments.

Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:151
+      if (SM.isInMainFile(RD->getMostRecentDecl()->getLocation())) {
+        if (Definition)
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;
  if (mostRecentInMainFile) {

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?

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list