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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits