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