kadircet accepted this revision. kadircet marked an inline comment as done. kadircet added a comment. This revision is now accepted and ready to land.
let's ship it! ================ Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:196 constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error; + constexpr static llvm::SourceMgr::DiagKind Warning = + llvm::SourceMgr::DK_Warning; ---------------- sammccall wrote: > kadircet wrote: > > nit: I know this isn't what the patch is about, but.... what about making > > these lambdas/functions of Out.diag with first parameter bound to DK_Error > > or DK_Warning? (or having them at an anonymous namespace instead of a > > member) > I'm not sure about the readability of lambdas here, if we wanted to do this I > think the simplest thing would just be to make them methods? > > Can we defer this question until after the branch cut? SG :+1: ================ Comment at: clang-tools-extra/clangd/ConfigFragment.h:129 + /// Legal values are "Build" or "Skip". + llvm::Optional<Located<std::string>> Background; + }; ---------------- sammccall wrote: > kadircet wrote: > > why not store the enum in here directly and generate diagnostics during the > > parsing stage? > > > > we seem to be doing this for syntactic errors, e.g. `expected a dictionary` > > and getting an unexpected enum value sounds similar to that? > Because then you have to do it twice for JSON vs YAML. You're right this > isn't really principled, and I think more data points will help us resolve it. > OK to defer this to after the branch cut? sure Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83768/new/ https://reviews.llvm.org/D83768 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
