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