[PATCH] D143906: [include-cleaner] Better support ambiguous std symbols

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:104
+  }
+  if (FName == "remove") {
+    if (FD->getNumParams() == 1)
----------------
nit: `else if`


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:110
+      // remove(ForwardIt first, ForwardIt last, const T& value);
+      return {Header("<algorithm>")};
+  }
----------------
we also need to account for exporters, similar to `findHeaders`. might be worth lifting that branch to a free-function.


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:174
                                            const PragmaIncludes *PI) {
+  if (auto Headers = specialStandardSymbols(S); !Headers.empty())
+    return Headers;
----------------
rather than doing this here and bailing out early, i think we should just augment the `Headers` below, after `locateSymbol` and `findHeaders` is run.

that way we'll make sure rest of the logic applies in the future without special casing (e.g. we've some fixmes for treating implementation locations for stdlib symbols as providers, in such a scenario this early exits would create non-uniformity).
it also implies we'll need to think about hints to attach here, which seems consistent with rest of the logic again (as we still assign hints to headers derived from stdlib recognizer).

so what about an alternative with a signature like: `llvm::SmallVector<Hinted<Header>> headersForSpecialSymbols(S, SM, PI);` then we can just initialize Headers with a call to it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143906