sammccall added a comment. This is basically the reverse of D89128 <https://reviews.llvm.org/D89128> + D89131 <https://reviews.llvm.org/D89131>.
I'm not sure if I like it, but I might be biased :-) It was put in place after I screwed up the type of a protocol message and we didn't detect it until someone reported that some setting wasn't respected in VSCode. A log message would have made it easier to debug, and *may* have led to us catching it, unsure. I'd probably lean toward fixing https://github.com/clangd/vscode-clangd/issues/134 where we *know* of bad clients, but not based on the possibility alone. WDYT? ================ Comment at: clang-tools-extra/clangd/Protocol.cpp:33 +template <typename T> +bool mapOptionalOrLog(llvm::json::ObjectMapper &O, llvm::StringLiteral Prop, + T &Out) { ---------------- I would call this tryMap for brevity ================ Comment at: clang-tools-extra/clangd/Protocol.cpp:36 + if (!O.mapOptional(Prop, Out)) { + elog("Unexpected value for optional field '{0}', ignoring", Prop); + Out = T{}; // Default-initialize Out to ensure we don't have a ---------------- On the one hand, logging a message seems better than doing this silently. On the other, if clients have this issue, these messages are likely to be frequent (at least every message) and not so helpful: unlike the current messages they don't include the context. ================ Comment at: clang-tools-extra/clangd/Protocol.cpp:37 + elog("Unexpected value for optional field '{0}', ignoring", Prop); + Out = T{}; // Default-initialize Out to ensure we don't have a + // half-constructed object. ---------------- this isn't necessarily the *field* default though! e.g. if we had `KeepFingerAttachedToHand = true` then setting it to 1 would result in `false`. I think this coincides for all the concrete callsites here. But i'm nervous we add a pattern that could bite us later. We could do something weird like default-allocate a T and swap it if things succeed. ================ Comment at: clang-tools-extra/clangd/Protocol.cpp:40 + } + return true; +} ---------------- not sure if returning always-true for chaining is helpful or misleading... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95229/new/ https://reviews.llvm.org/D95229 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
