[PATCH] D153769: [clangd] Implement the 'Organize Imports' source action. Fix include-cleaner findings in batch.

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked an inline comment as done.
VitaNuo added a comment.

Respond to comments.



================
Comment at: clang-tools-extra/clangd/refactor/tweaks/OrganizeImports.cpp:56
+    return false;
+  Range PreambleRange;
+  PreambleRange.start =
----------------
kadircet wrote:
> VitaNuo wrote:
> > kadircet wrote:
> > > kadircet wrote:
> > > > relying on `MainFileIncludes` for preamble range is not correct in general. we can have includes way down inside the main file that'll be part of this vector, but not preamble (also it's not sorted explicitly).
> > > > 
> > > > we should instead use `ComputePreambleBounds` (nit: we can also store it in ParsedAST, instead of relexing the file one more time with each CodeAction request)
> > > Having a comment for reason would also be helpful, something like `To accommodate clients without knowledge of source actions, we trigger without checking code action kinds when inside the preamble region`.
> > > nit: we can also store it in ParsedAST
> > 
> > Seems like the data is already there, I just need to expose it.
> > > nit: we can also store it in ParsedAST
> > Seems like the data is already there, I just need to expose it.
> 
> not really, we calculate bounds when building a `ParsedAST` but we don't store it anywhere.
I was referring to https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/PrecompiledPreamble.cpp#L580. We store the `PreambleData` and that seems to store the `PrecompiledPreamble` which seems to know its bounds


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153769