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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
