Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eldoc support #166

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Eldoc support #166

wants to merge 4 commits into from

Conversation

ikirill
Copy link

@ikirill ikirill commented Jan 20, 2015

Following #162, this PR is smaller, and only returns information about the type of a given expression.

irony-exprtype is an emacs-lisp function returns an association list of properties for the expression at point (or in region, if a region is active). This can be transformed to whatever is suitable for display.

@ikirill ikirill force-pushed the eldoc-support branch 2 times, most recently from a99249d to 22fd810 Compare January 20, 2015 03:25
The bulk of this commit is slightly dull code for printing information
about libclang cursors in sexp form.
@Sarcasm
Copy link
Owner

Sarcasm commented Jan 20, 2015

Hello ikirill,

I will have to test this evening at home but the common thread between all the pull requests you have provided and where I'm reluctant is the complexity of the resulting code.

See that I have 26 pages to scroll when looking at https://github.com/Sarcasm/irony-mode/pull/166/files scares the shit out of me.

Thinks like that: https://github.com/Sarcasm/irony-mode/pull/166/files#diff-4009c1159289a1fabe776a2396944394R10 should be avoided if possible. It is just a maintenance burden to keep this kind of mapping in sync with the elisp part, and until now it hasn't been needed. Maybe one day it will but right now I'd like to think it is not.

A "long time" ago for completion I was trying to provide exactly what libclang was providing, mapping kinds to elisp symbols and stuff like that. By operating some transformation on the irony-server side, I don't think I have really limited what the Elisp clients can do but I have greatly simplified the implementation (and sometime performances, though there is still a lot of work to do here).

irony-exprtype is an emacs-lisp function returns an association list of properties for the expression at point (or in region, if a region is active). This can be transformed to whatever is suitable for display.

I would prefer that you stick to you use case for now. If what you want to do is provide the current expression/scope boundaries to perform semantic kill of expression, I would like to see that, if what you want is to provide irony-eldoc context information, I would like to see that as well.

Also if you could provide a sample code and what is the output for the expression at point that would be great. I know I can get this by testing the pull request but maybe there are things to discuss before implementing it.

I feel very bad refusing all your hard work, I hope you continue to work with me on improving irony-mode.

@ikirill
Copy link
Author

ikirill commented Jan 21, 2015

See that I have 26 pages to scroll when looking at https://github.com/Sarcasm/irony-mode/pull/166/files scares the shit out of me.

Most of it is C++-style boilerplate, and a huge chunk is just parsing the output of libclang functions. The code that decides what gets printed is in Irony.cpp, everything else is just print statements.

I would prefer that you stick to you use case for now. If what you want to do is provide the current expression/scope boundaries to perform semantic kill of expression, I would like to see that, if what you want is to provide irony-eldoc context information, I would like to see that as well.

No, this is strictly for eldoc. I really don't want to have to modify the server whenever I want to display something differently in eldoc, and if libclang provides some information that is helpful to me in lisp, I want to be able to use it.

Also, I want to point out two things.

First, there are many different things things that can be seen in C++ code, and providing libclang's output to emacs-lisp code in the form of expressive association lists is a clean way to get emacs-lisp code to understand what is actually there in the file. You can return an opaque string and just tell emacs-lisp to display that, but that's just crufty. Libclang doesn't really make any strong promises about what the AST will look like.

Second, C++ doesn't really change all that much, libclang's interface should be reasonably stable. I don't know what version you're targeting, though. If some enum value is unknown, it will just return the unknown symbol, with no harm done. I generated the switch statements automatically from Index.h.

Third, you can't just rely on completion returning you the right results, it's not that clever. For example, for a dependent type in a call expression, there will be no completion for the call expression itself, and there will be several completion strings for the function being called. A reasonably clean way of handling this is to tell emacs-lisp code that there is some complexity in the return value, and let it handle the complexity however it wants. For example:

(exprtype
 (bounds 832 836)
 (kind . CallExpr)
 (type (kind . Dependent) (spelling . "<dependent type>"))
 (call
  (bounds 832 833)
  (kind . OverloadedDeclRef)
  (spelling . "h")
  (overloaded
   (completion (comment . "docstring for h(X).")
               (priority . 50)
               (chunks (ResultType . "X") "h(" (Placeholder . "const X &x")")"))
   (completion (comment . "docstring for h<T>")
               (priority . 50)
               (chunks (ResultType . "T") "h(" (Placeholder . "const T &x")")"))))
 (args (834 . 835)))

This is a fairly uniform structure: everything is an association list.

Also if you could provide a sample code and what is the output for the expression at point that would be great. I know I can get this by testing the pull request but maybe there are things to discuss before implementing it.

ikirill@db6026b#diff-90f3661683daeb04226a61e026a6f7afR477

I use this file for testing.

    #include <vector>
    #include <string>
    #include <algorithm>
    // #include <boost/geometry.hpp>
    // #include <boost/geometry/geometries/point_xy.hpp>
    // #include <boost/geometry/geometries/polygon.hpp>
    // using namespace boost::geometry;
    // using namespace std;
    using std::string;
    using std::vector;

    /** docstring for f */
    void f(string x);
    /** docstring for f(int, int) */
    void f(int x, int y);
    /** docstring for f(double, double) */
    void f(double x, double y);
    /** docstring for f(string, string) */
    void f(string x, string y);
    /** docstring for g2(string&). */
    void g2(string& x) { }

    /** docstring for h<T> */
    template <typename T>
    T h(const T& x);
    /** docstring for X. */
    struct X { };
    /** docstring for h(X). */
    X h(const X& x);
    // Overloading cursors
    /** docstring for h2(T) */
    template <typename T>
    void h2(T t) {
      h(t) + h(t) + h(t) * h(t);
    }
    struct Y { };
    /** docstring for h(Y). */
    Y h(const Y& y);

    /** SomeStruct docstring */
    template <typename Type1, typename Type2 = float>
    struct SomeStruct { Type1 elt1; Type2 elt2; };
    /** docstring for g
        \param x g-docstring/param-x
     */
    void g(/** g-arg-x-1 */ string /** g-arg-x-2 */ x /** g-arg-x-3 */ ) {
      /** docstring y */
      vector<float> y;
      y = vector<float>(10, 0.1);
      sort(y.begin(), y.end());
      sort(y.begin(), y.end());
      f(x);
      f(x, /* comment */ x);
      g(x); // x is bound to a temporary
      g2(x); // x is not bound to a temporary
      f(x,
        x);
      h(x);
      /** SomeStruct ss1 docstring */
      SomeStruct<double> ss1;
      /** SomeStruct ss2 docstring */
      SomeStruct<double, double> ss2;
      // model::d2::point_xy<int> p1(1, 1), p2(2, 2);
      // std::cout << "Distance p1-p2 is: " << distance(p1, p2) << std::endl;
      int z = 1, w = 2, p = z + w + w + NULL;
    }

    struct X;

    // Local Variables:
    // eval: (when (fboundp (quote flycheck-mode)) (flycheck-mode -1))
    // End:

It's a bit tedious to write library bindings in a non-automated way, but definitely not hard. The build fails right now, because it's using some old version of libclang, I think.

The fact that you can select some arbitrary expression/region (not just a symbol) and find out its type (technically, the type of the innermost enclosing expression) is quite nice, I think.

This PR being 1k lines long and tied to a recent version of libclang (it needs getTypeSpelling, I think most other things can be commented/ifdef'd out easily) is unfortunate. You can even comment out getTypeSpelling and just print nil instead of type spellings.

@ikirill
Copy link
Author

ikirill commented Jan 21, 2015

It is just a maintenance burden to keep this kind of mapping in sync with the elisp part

But there is no elisp part: either elisp gets a symbol it understands, and does something useful with it, or it gets a symbol it doesn't understand and just ignores it.

@Sarcasm
Copy link
Owner

Sarcasm commented Jan 21, 2015

First, there are many different things things that can be seen in C++ code, and providing libclang's output to emacs-lisp code in the form of expressive association lists is a clean way to get emacs-lisp code to understand what is actually there in the file. You can return an opaque string and just tell emacs-lisp to display that, but that's just crufty. Libclang doesn't really make any strong promises about what the AST will look like.

I do not agree with the statement in bold. I used to return some association list from, see older versions of irony-mode (https://github.com/Sarcasm/irony-mode/blob/v0.0.1-alpha/server/plugins/CodeCompletion.cpp#L399,

const char *completionCursorKindIdentifier(CXCursorKind cursorKind)
) I changed my mind and decided that provided less was better. It's has been highly beneficial to make these changes, I found out it was easier to work with. It was easier to provide a simple API in irony mode that in turn was easier to use for multiple bindings (completion at point, company, auto-complete, ...). Right now I'm thinking about changing the dumping of the results to something else than Sexpr, I'm not afraid to do it.

If you want to have access to the libclang AST in Emacs, irony-mode isn't the right tool (in the foreseeable future).

@ikirill
Copy link
Author

ikirill commented Jan 21, 2015

If you want to have access to the libclang AST in Emacs, irony-mode isn't the right tool (in the foreseeable future).

This patch does not expose the AST to emacs code, it returns enough information to display type at point/region. Like I said, I don't want to have to modify irony-server every time something should be displayed differently, and I would like emacs code to understand, roughly, what is being displayed and why.

I used to return some association list from...

That is, in fact, a better design. It means that other people can reuse the output of your API in ways that you have not foreseen exactly.

@Sarcasm
Copy link
Owner

Sarcasm commented Jan 21, 2015

When I say I'm against the exposing the AST I wanted to say that I'm against exposing the libclang internals.

I used to return some association list from...

That is, in fact, a better design. It means that other people can reuse the output of your API in ways that you have not foreseen exactly.

No it is too difficult to provide something stable based on this. At one time I returned things like Clang "typed text " as a cons (:typed_text . "foo"), then it was too messy and unreadable, and took way too much size to be that verbose so I reduced to (?t . "foo") or even "foo", I don't remember exactly. Anyway, what was supposed to give more power to the clients just gave more maintenance burden. I'm quite satisfy with the data I return now.

I think we have reached, the end of the debates. I'm personally convinced that exposing libclang is a bad idea, and that is not the road I want to take for irony. If we can't agree on this I suggest we continue on separate roads, you fork irony-mode, make your another an elisp binding to libclang, whatever but I will not accept code that do not offer a precise use-case in irony-server for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants