[PATCH] D51438: [clangd] Run SignatureHelp using an up-to-date preamble, waiting if needed.

[Yvan Roux via cfe-commits]

Seems that it was fixed earlier today.

Thanks,
Yvan

On Tue, 18 Sep 2018 at 19:31, Yvan Roux <yvan.roux at linaro.org> wrote:
>
> Hi Sam,
>
> It took a very long time to identify it, but this commit broke ARMv7
> bots, where this test hangs.  Logs are available here (initial ones
> are too old):
>
> http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/3685/steps/ninja%20check%201/logs/stdiohttp://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/3685/steps/ninja%20check%201/logs/stdio
>
> Thanks,
> YvanOn Thu, 30 Aug 2018 at 17:15, Sam McCall via Phabricator via
> cfe-commits <cfe-commits at lists.llvm.org> wrote:
> >
> > sammccall added inline comments.
> >
> >
> > ================
> > Comment at: clangd/TUScheduler.cpp:474
> > +    llvm::unique_function<void(std::shared_ptr<const PreambleData>)> Callback) {
> > +  if (RunSync)
> > +    return Callback(getPossiblyStalePreamble());
> > ----------------
> > ilya-biryukov wrote:
> > > It seems we could remove the special-casing of `RunSync` and still get correct results (the Requests queue is always empty).
> > > But feel free to keep it for clarity.
> > Right, of course :-)
> > Replaced this with an assert before we write to the queue.
> >
> >
> > ================
> > Comment at: clangd/TUScheduler.h:123
> > +  /// Controls whether preamble reads wait for the preamble to be up-to-date.
> > +  enum PreambleConsistency {
> > +    /// The preamble is generated from the current version of the file.
> > ----------------
> > ilya-biryukov wrote:
> > > Maybe use a strongly-typed enum outside the class?
> > > `TUScheduler::Stale` will turn into `PreambleConsistency::Stale` at the call-site. The main upside is that it does not pollute completions inside the class with enumerators.
> > >
> > > Just a suggestion, feel free to ignore.
> > Yeah, the downside to that is that it *does* pollute the clangd:: namespace. So both are a bit sad.
> >
> > This header is fairly widely visible (since it's included by clangdserver) and this API is fairly narrowly interesting, so as far as completion goes I think I prefer it being hidden in TUScheduler.
> >
> >
> > Repository:
> >   rL LLVM
> >
> > https://reviews.llvm.org/D51438
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits