tom-anders added a comment. In D137909#3930043 <https://reviews.llvm.org/D137909#3930043>, @sammccall wrote:
> What do you think about `$name(payload)^` for the general case, with > `$(payload)^`, `$name`, and `^` as abbreviated forms? I don't think the lack > of brackets on **names** hurts a lot if they stay simple, and if they're > "just labels" that don't have to carry arbitrary payloads then they can be > simple. Thanks for the detailed feedback, I think this looks perfect! I adapted the code and docs accordingly (I added some comments to the parts I'm not sure about) In D137909#3930047 <https://reviews.llvm.org/D137909#3930047>, @sammccall wrote: >> D137894 <https://reviews.llvm.org/D137894> > > this looks cool! I left a couple of comments, mentioning here because phab > won't notify for comments on drafts. Thanks! I replied to one of them :) ================ Comment at: clang-tools-extra/clangd/unittests/Annotations.h:30 + struct Position : clangd::Position { + llvm::Optional<llvm::StringRef> Payload; ---------------- I thought about just using `std::pair<clangd::Position, llvm::Optional<llvm::StringRef>> instead, but that is pretty verbose and unreadable here IMO. Another nice thing about subclassing here is that we can keep the API stable and don't have to introduce an additional `pointWithPayload()` getter like in the base class. ================ Comment at: llvm/include/llvm/Testing/Support/Annotations.h:61 size_t End = 0; + llvm::Optional<llvm::StringRef> Payload; ---------------- One could argue that this should just be `llvm::StringRef`. Right now, `$name^` results in `llvm::None` for the payload, while `$name()^` will yield an empty string. Do you think they should just both be empty strings instead? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137909/new/ https://reviews.llvm.org/D137909 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
