[PATCH] D139921: [include-cleaner] Ranking of providers based on hints

[Kadir Cetinkaya via Phabricator via cfe-commits]

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