[PATCH] D145773: [clangd] UnusedIncludes: Strict config now uses the include-cleaner-library implementation.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet accepted this revision.
kadircet added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:438
+                         .map("Experiment",
+                              Config::IncludesPolicy::Strict) // for backward
+                                                              // compatibility
----------------
hokein wrote:
> kadircet wrote:
> > i think we should at least be emitting a diagnostics to encourage people for moving back to strict, so what about something like:
> > ```
> > if (F.UnusuedIncludes) {
> >   auto Val = compileEnum....; // only for Strict and None
> >   if (!Val && **F.UnusedIncludes == "Experiment") {
> >        diag(Warning, "Experiment is deprecated for UnusedIncludes, use Strict instead.", F.UnusedIncludes.Range);
> >        Val = Config::IncludesPolicy::Strict;
> >   }
> > }
> > ```
> I thought it was not worth a diagnostic because this flag was introduced recently, and we have never advertised it to open-source users.
> 
> But the flag is in the recent clangd16 release, so it probably justifies the value. 
> 
> BTW, looks like we forgot to update the release notes for clangd16, https://github.com/llvm/llvm-project/blob/release/16.x/clang-tools-extra/docs/ReleaseNotes.rst#improvements-to-clangd is empty.
> 
> But the flag is in the recent clangd16 release, so it probably justifies the value.

right.

> BTW, looks like we forgot to update the release notes for clangd16, https://github.com/llvm/llvm-project/blob/release/16.x/clang-tools-extra/docs/ReleaseNotes.rst#improvements-to-clangd is empty.

yeah, i've aslo noticed that will take a look. but regarding this feature, I don't think we should be advertising `Experiment` there, right?


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:769
-      Cfg.Diagnostics.UnusedIncludes == Config::IncludesPolicy::Strict
-          ? computeUnusedIncludes(AST)
-          : Findings.UnusedIncludes,
----------------
hokein wrote:
> kadircet wrote:
> > can you also delete `computeUnusedIncludes` and its friends (also from the tests)?
> this is in plan, but in a separate patch, https://reviews.llvm.org/D145776
as mentioned on that patch, i think it's better to land them in single step, to make sure we can revert a single patch if we want the old behavior rather than multiple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145773