sammccall added inline comments.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:65
+ llvm::ArrayRef<SymbolReference> MacroRefs,
+ const Includes &, const PragmaIncludes *PI,
+ const SourceManager &SM, HeaderSearch &HS);
----------------
kadircet wrote:
> nit: name the `Includes` parameter. preferably `MainFileIncludes` (or
> `ProvidedIncludes`)?
Added a name for consistency, but I don't think the name is actually useful
here.
("MainFile" is inconsistent with the other parameters, and "Provided" is
redundant for an input parameter)
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:77
+ bool Satisfied = false;
+ for (const Header &H : Providers) {
+ if (H.kind() == Header::Physical && H.physical() == MainFile)
----------------
kadircet wrote:
> nit: maybe leave a comment here for skipping `Header`s we've seen before.
> there's a quite good chance that we'll see same provider showing up multiple
> times, skipping processing might be helpful (later on we'll probably need to
> cache the analysis results for diagnostics purposes).
I think you're talking about a performance optimization using a cache?
We still need to process each header to compute `Satisfied`. So at most we're
replacing a trivial comparison + 2 hashtable lookups (`Inc.match` and
`Used.insert`) with one cache lookup. (The inner loop count is ~always <=1).
Happy with such a change if it improves a benchmark of course but my
expectation is that it *wouldn't* (small constant factor on a fairly fast
operation) so a FIXME seems premature here.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:111
+ if (auto Edit = HI.insert(Insert.trim("<>\""), Insert.starts_with("<"))) {
+ if (PendingInsert && Edit->getOffset() == PendingInsert->getOffset()) {
+ PendingInsert = tooling::Replacement(
----------------
kadircet wrote:
> this logic makes me feel a little bit uneasy, as we're relying on
> alphabetical sorting of `Results.Missing`, which isn't just the filenames but
> also contains `<` or `"` at the beginning.
> clang-format has weird include categories but I think it never mixes `"`
> includes with `<` includes. so we're probably safe here.
> but it might still be safer to just keep a map of offsets to edits, up to
> you. (having a comment at the very least would be nice)
TL;DR: replaced implementation with clang-format, tweaked signature
---
Good catch, this is completely wrong, I should capture all the edits,
stable_sort them, and then fold together.
But this got me thinking that actually this doesn't give the correct relative
order of inserted headers: they'll just be in whatever order I start with. I
switched to using libFormat instead - I can't really understand from the
implementation how it solves this problem, but it probably does, and if not
it's the right place to fix it.
This had some knock-on effects on the API: no more messing with MemoryBuffer
etc.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D139013/new/
https://reviews.llvm.org/D139013
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
