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

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev 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.
----------------
sammccall wrote:
> kbobyrev wrote:
> > sammccall wrote:
> > > 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?
> > Yes, this is for cases like
> > 
> > ```
> >       {
> >           "enum class Color;",
> >           "enum class Color {}; Color c;",
> >       },
> > ```
> > 
> > I think the problem here is that if we see the definition of the enum, it should be the "ground truth" for the usage, forward declarations are not really useful in this case. It's not much different from the class but I just wanted to handle it separately, not sure if it's OK to merge these two changes into one patch.
> The heuristic seems plausible, though there are counterexamples like:
> 
> ```
> "enum Color;"
> "void foo(Color); enum Color {};"
> ```
> 
> My main argument would be that we have a general policy here: conservatively consider all redecls as used, rather than trying to work out which is best. It's not clear why enums are special enough that we should hack around them without rethinking the policy.
> 
> (I think `VisitEnumDecl` is different - our general policy is that decls *aren't* references to forward-decls, and in enums' case this is violates our meta-policy of "be conservative, consider everything that might be a use").
This and our offline discussion makes total sense, I will delete it from the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112209