Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

On Thu, 2020-06-04 at 13:06 +0300, Kadir Çetinkaya wrote: > Hi Mikael, > > sent out 4f4a8ae72e95f2c7fa5e4ca56dd6b1a83a304680, please let me know > if it helps! Hi, Yes, now it's silent. Thank you! /Mikael > > On Thu, Jun 4, 2020 at 12:40 PM Mikael Holmén via Phabricator < > revi...@reviews.l

Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

Hi Mikael, sent out 4f4a8ae72e95f2c7fa5e4ca56dd6b1a83a304680, please let me know if it helps! On Thu, Jun 4, 2020 at 12:40 PM Mikael Holmén via Phabricator < revi...@reviews.llvm.org> wrote: > uabelho added inline comments. > > > > Comment at: clang-tools-extra/clangd/CodeComple

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

uabelho added inline comments. Comment at: clang-tools-extra/clangd/CodeComplete.cpp:1034 const PreambleData &Preamble; - const PreamblePatch &Patch; + llvm::Optional Patch; llvm::StringRef Contents; Hi! When compiling with gcc 7.4 I see a warning that I

Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

managed to reproduce the issue. sent out https://reviews.llvm.org/D80988 for a fix. On Tue, Jun 2, 2020 at 12:08 PM Sam McCall wrote: > On Tue, Jun 2, 2020 at 10:38 AM Kadir Çetinkaya > wrote: > >> Hi Jan, >> >> I don't think there's much point in running ReplayPreamble with an empty >> preambl

Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

On Tue, Jun 2, 2020 at 10:38 AM Kadir Çetinkaya wrote: > Hi Jan, > > I don't think there's much point in running ReplayPreamble with an empty > preamble, but this should already be a no-op as there can't be any includes > inside the preamble region if size is 0. > > I can't seem to reproduce a fa

Re: [PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

Hi Jan, I don't think there's much point in running ReplayPreamble with an empty preamble, but this should already be a no-op as there can't be any includes inside the preamble region if size is 0. I can't seem to reproduce a failure with the root causes you've provided. Even when ReplayPreamble:

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

jkorous added a comment. Hi @kadircet! I am investigating a failure of `PatchesAdditionalIncludes` test that we got internally. It's a failed assert in `ReplayPreamble::replay`. Our clangd source code is for all practical purposes identical to upstream version but not so with clang source. Speci

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

This revision was automatically updated to reflect the committed changes. Closed by commit rGb742eaa32121: [clangd] Handle additional includes while parsing ASTs (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https:

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet updated this revision to Diff 266215. kadircet added a comment. - Fix windows build bots in the face of `#import` directives Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https://reviews.llvm.org/D77644 Files: clang-tools-ext

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet updated this revision to Diff 266174. kadircet added a comment. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https://reviews.llvm.org/D77644 Files: clang-tools-extra/clangd/CodeComplete.cpp clang-tools-extra/clang

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Sorry for the delay. I think this is good though I have a nagging feeling I don't understand all the implications yet :-) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet updated this revision to Diff 261745. kadircet marked an inline comment as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https://reviews.llvm.org/D77644 Files: clang-tools-ex

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet marked 8 inline comments as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:280 const PreambleData &Baseline) { + assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!"); // F

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

sammccall added a comment. This is excellent, a pretty nice model in the end! Good job puzzling it out, when we discussed the idea in the past I imagined this part would be a pile of hacks. Thanks for the offline help understanding this, too... Comment at: clang-tools-extra/c

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/Preamble.h:115 + /// mutate include depths. + void patchPreambleIncludes(IncludeStructure &BaselineIncludes) const; + i am not happy about this inter

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet added a comment. this is ready for review now, to explain the flow: While creating a patch we also record remaining includes from Baseline left in Modified, as the new ones will be put into the patch already. That way ParsedAST can keep track of latest state of the preamble + main file

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet updated this revision to Diff 260228. kadircet added a comment. - Use PreamblePatch - Add more tests - Handle deleted includes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https://reviews.llvm.org/D77644 Files: clang-tools-e

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet planned changes to this revision. kadircet added a comment. this doesn't handle source locations correctly in presence of deleted headers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77644/new/ https://reviews.llvm.org/D77644

[PATCH] D77644: [clangd] Handle additional includes while parsing ASTs

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, mgrang, jkorous, MaskRay, ilya-biryukov. Herald added a project: clang. Depends on D77392 . Enables building ASTs with stale preamble