[PATCH] D26752: [include-fixer] Refactor include fixer to be usable as a plugin

[Benjamin Kramer via cfe-commits]

bkramer added inline comments.


================
Comment at: include-fixer/IncludeFixer.cpp:136
+
+    auto Begin = StartOfFile.getLocWithOffset(Placed.getOffset());
+    auto End = Begin.getLocWithOffset(Placed.getLength());
----------------
ioeric wrote:
> hokein wrote:
> > ioeric wrote:
> > > bkramer wrote:
> > > > hokein wrote:
> > > > > I have a concern that `Placed` here might be not the replacement for inserting the new header, becuase the `Reps` returned from `createIncludeFixerReplacements` may have some replacements for cleanup.
> > > > > 
> > > > > To make it more correct, maybe we can check whether `Placed.getReplacementText()` is equal to `"#include" + Context.getHeaderInfos().front().Header`?
> > > > I don't think that will work. We do want to put the replacement into the right position so we have to apply the full cleanup, right? Just comparing with the header path doesn't work because the cleanup is larger than that.
> > > Yep, `createIncludeFixerReplacements` also sorts `#include`s with `formatReplacements`, so the returned replacements can be larger than header insertion.
> > I see. What I concern is that the behavior won't be correct if there are multiple replacements in `*Reps`. But looks like if we pass only one Replacement for inserting the new header to `createIncludeFixerReplacements`, the `Reps` only has a `Replacement` inside, which is correct.
> > 
> > Maybe add an `assert` here?
> This is implementation detail; the interface does not guarantee that. And I guess there might still be cases where more than one replacement is generated.
We'll see if this comes up. The current implementation wouldn't deal well with multiple replacements as it would format them as alternatives. I'll do the assert for now.


https://reviews.llvm.org/D26752