kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land.
LGTM thanks! ================ Comment at: clang-tools-extra/clangd/FormattedString.cpp:365 + llvm::StringRef Marker = ""; + if (C.Preserve && C.Kind == Chunk::InlineCode) + Marker = "`"; ---------------- sammccall wrote: > kadircet wrote: > > should we rather use `renderInlineBlock` here ? because in presence of > > backticks inside the C.Contents it might become confusing e.g: > > > > ``` > > this is`foo(`x`)` > > ``` > > > > this would become: > > ``` > > this is `foo(``x``)` > > ``` > > > > and instead of keeping a marker maybe just: > > > > ``` > > if (Preserve && ..) > > OS << Sep << "`" << C.Contents << "`"; > > else > > OS << Sep << C.Contents; > > ``` > > rather use renderInlineBlock > > I don't think humans deal with escaping better than with ambiguity - those > two examples seem equally confusing to me :-\ > > I'd suggest better is to pick another marker that's unused in the chunk: > ``` > this is 'foo(`x`)' > ``` > > Happy to do that in this patch if you like it. > instead of keeping a marker maybe just: > > > instead of keeping a marker maybe just > > Happy to do that for now if you like, but I don't think it's much clearer. I > factored it this way because I think the marker is likely to have more > options in future (bold spans, avoiding conflicts) and wanted to hint at that. > I don't think humans deal with escaping better than with ambiguity - those > two examples seem equally confusing to me :-\ > I'd suggest better is to pick another marker that's unused in the chunk: > this is 'foo(`x`)' > Happy to do that in this patch if you like it. Yeah that looks like a better approach, feel free to leave a fixme as well. As I believe this should be a rather rare occurrence and we can take care of it once it becomes a real issue or someone has time. > > instead of keeping a marker maybe just > Happy to do that for now if you like, but I don't think it's much clearer. I > factored it this way because I think the marker is likely to have more > options in future (bold spans, avoiding conflicts) and wanted to hint at that. OK makes sense let's keep it that way. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79142/new/ https://reviews.llvm.org/D79142 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
