kadircet accepted this revision.
kadircet added a comment.
This revision is now accepted and ready to land.
thanks LGTM!
let me know if I should land this for you.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1054
// editor, as they might be long.
- if (ReturnType) {
+ if (ReturnType)
// For functions we display signature in a list form, e.g.:
----------------
can you add braces around here?
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1074
+ if (Type && !ReturnType && !Parameters)
+ // Don't print Type after Parameters or ReturnType
+ Output.addParagraph().appendText("Type: ").appendCode(*Type);
----------------
can you move the comment above the if and also include `as this will just
duplicate the information`.
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1065
+ if (Parameters && !Parameters->empty()) {
+ Output.addParagraph().appendText("Parameters: ");
----------------
lh123 wrote:
> kadircet wrote:
> > it's a subtle invariant that we only have parameters for functions (which
> > has a return type) or constructor/destructor/conversion-operators (which
> > doesn't have either a type or a return type).
> >
> > I think we should assert on `Type` not being present here, as otherwise we
> > would probably duplicate parameters in both places. can you also add a
> > condition around `!Type` and have a comment saying ` // Don't print
> > parameters after Type, as they're likely to be mentioned there.`
> We cannot assert here because the `function type` has both `ReturnType`,
> `Type`, and `Parameters`. I think we only need to not print `Type` when
> `ReturnType` or `Parameters` are present.
sorry by asserting i meant having it inside the check. this LG, thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114621/new/
https://reviews.llvm.org/D114621
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
