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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.


================
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; }
----------------
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`)


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:55
+  }
+  Decl &declaration() const { return *std::get<Declaration>(Storage); }
+
----------------
nit: I think it would be better to spell out the types here (and above), rather than rely on what `Kind` would translate into


================
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;
 };
----------------
nit: any particular reason for dropping const here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136710