[PATCH] D112209: [clangd] IncludeCleaner: Complicated rules for enum usage

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:38
   bool VisitTagType(TagType *TT) {
+    // For enumerations we will require only the definition if it's present and
+    // the underlying type is not specified.
----------------
I don't understand this special case.
It seems you're trying to avoid requiring too many redecls of a referenced type. Why is this important, and different from e.g. Class?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:93
 
+  // Require redeclarations only for definitions and only when the underlying
+  // type is specified.
----------------
This says what, rather than why.

Rather maybe:
``` 
Enums may be usefully forward-declared as *complete* types by specifying an underlying type.
In this case, the definition should see the declaration so they can be checked for compatibility.
```


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:96
+  bool VisitEnumDecl(EnumDecl *D) {
+    if (D != D->getDefinition() || !D->getIntegerTypeSourceInfo())
+      return false;
----------------
D->isThisDeclarationADefinition() is more idiomatic for the first part


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:98
+      return false;
+    for (const auto *Redecl : D->redecls())
+      Result.insert(Redecl->getLocation());
----------------
rather add(D) here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112209