[PATCH] D136710: [include-cleaner] Add the missing parts of Symbol and Header clases.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked an inline comment as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:49
 
+  Kind kind() const { return static_cast<Kind>(Storage.index()); }
+  bool operator==(const Symbol &RHS) const { return Storage == RHS.Storage; }
----------------
kadircet wrote:
> nit: how about implementing this with `std::visit`? it's definitely more code, but at least makes sure:
> - we won't depend on the order in `Kind` matching the `Storage`, and
> - we'd get a hard compiler error when a new variant is introduced without proper handling here
> 
> it would look like:
> ```
> Kind kind() const {
>   struct KindTranslator {
>       Kind operator()(Decl *) { return Declaration; }
>       ...
>   };
>   return std::visit(KindTranslator{}, Storage);
> }
> ```
> 
> but not feeling so strongly about it, so feel free to keep as is if needed (I don't think we'll add tons of more kinds later on, so the chances of introducing bugs is small. but using new language/library constructs sounds fun).
> (same for `Header`)
This is a bunch more code to read, is no longer a trivial accessor when compiled. It does look canonical, but I think this speaks more to std::variant being not great (we don't have a lot of experience with it yet).

It's not *that* much more safe, the error `enum {A, C, B}` can still be expressed as 
```
Kind operator()(A*)  { return A; }
Kind operator()(B*)  { return A; /*oops*/ }
```
It's silghtly more locally obvious, but it's buried in a bunch of boilerplate.

(It also leans into addressing variants with types, as I mentioned below)


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:55
+  }
+  Decl &declaration() const { return *std::get<Declaration>(Storage); }
+
----------------
kadircet wrote:
> nit: I think it would be better to spell out the types here (and above), rather than rely on what `Kind` would translate into
This seems backwards to me: the compiler will check that the type is correct, but can't check that the alternative we're fetching matches the accessor we're defining.
This version places `declaration()` and `Declaration` next to each other so such a mistake is obvious.

(As a design question, I think we should to name variants rather than access them with types when this is ergonomic, e.g. it's perfectly reasonable to model header=physical/verbatim/stdlib as `variant<FileEntry*, StringRef, StringRef>`. Languages that take variants seriously tend to prefer this)


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:60
+  // Order must match Kind enum!
+  std::variant<Decl *, tooling::stdlib::Symbol> Storage;
 };
----------------
kadircet wrote:
> nit: any particular reason for dropping const here?
Oh, the constructor was nonconst so I assumed const was an oversight. Changed to const everywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136710