kadircet added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:133
+
+ Header(std::in_place_t, decltype(Storage) Sentinel)
+ : Storage(std::move(Sentinel)) {}
----------------
sammccall wrote:
> this is a bit confusing... kind of the opposite of what `std::in_place` means?
> (All of the other constructors build the state in-place in some sense, but
> this one doesn't)
>
> Maybe just a private `struct Sentinel{};` instead of in_place_t?
>
> Symbol does this without any sentinel at all, if that's causing ambiguity
> then it might be worth fixing both to be the same.
without a disambiguation tag we get an ambigious conversion when we have a
constructor call that looks like `Header("foo.h")` (whether to use
Header(StringRef) or Header(variant<StringRef>), not sure if it's fine for an
"inaccessible" constructor to participate in overload resolution, nor how
variant can be viable here despite needing a double implicit conversion from
stringref to variant)
This didn't bite us with Symbol yet because constructor takes a `Decl&` but
variant stores a `Decl*`, and for Macro case we always explicitly initialized
with spelling of `Macro`.
You're right about usage of inplace_t though, I was confused. using a sentinel
tag instead.
================
Comment at: clang-tools-extra/include-cleaner/lib/Types.cpp:114
+ case Header::Physical:
+ return physical() < RHS.physical();
+ case Header::Standard:
----------------
sammccall wrote:
> I think comparing pointers is going to give inconsistent results, and would
> suggest comparing Name (without trying to normalize, just for at least
> reproducibility across identical runs)
i was rather worried about correctness of a name based match, e.g. we can have
the same FileEntry pointers for `foo.h` and `bar/foo.h` but i guess it's OK to
assume that a FileManager won't be accessed in the middle of a comparison
sequence.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139921/new/
https://reviews.llvm.org/D139921
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
