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