hokein added inline comments.
================ Comment at: include-fixer/IncludeFixer.cpp:136 + + auto Begin = StartOfFile.getLocWithOffset(Placed.getOffset()); + auto End = Begin.getLocWithOffset(Placed.getLength()); ---------------- 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? https://reviews.llvm.org/D26752 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
