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

Sorting completion candidates based on clang priority #519

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion irony-completion.el
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,10 @@ displayed when a derived class overrides virtual methods."
(cl-case style
(case-insensitive "case-insensitive")
(smart-case "smart-case")
(t "exact"))))
(t "exact"))
(cl-case irony-candidates-clang-sorting
(alphabetical "alphabetical")
(t "alphabetical-clang"))))
:update irony--server-query-update)

(defun irony--candidates-task (&optional buffer pos prefix style)
Expand Down
11 changes: 11 additions & 0 deletions irony.el
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,17 @@ Larger values can improve performances on large buffers.
If non-nil, `w32-pipe-buffer-size' will be let-bound to this value
during the creation of the irony-server process.")

(defcustom irony-candidates-clang-sorting nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of vocabulary, I think I would do something like:

sorting-method: priority, alphabetical.

I would not allow nil, but instead produce an error.
nil seems to imply this is the soundest default value, which I am not yet sure.
A more neutral approach is to have enumeration value, and if one make more sense than the other, the defcustom will be updated accordingly.
I'm mostly thinking about a future 3rd option coming up, that would be the best default value.

"Determine the sorting strategy for completion candidates list

If set to nil, clang priority will be used on top of alphabetical sorting when completing to propose context-aware and more-likely completions.

If set to `alphabetical', clang priority will be ignored and candidates will be sorted alphabetically only."
:type '(choice
(const :tag "Alphabetical + clang priority" nil)
(const :tag "Alphabetical only" alphabetical)))



;;
;; Public/API variables
Expand Down
1 change: 1 addition & 0 deletions server/src/Command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ Command *CommandParser::parse(const std::vector<std::string> &argv) {
case Command::Candidates:
positionalArgs.push_back(StringConverter(&command_.prefix));
positionalArgs.push_back(PrefixMatchStyleConverter(&command_.style));
positionalArgs.push_back(StringConverter(&command_.sorting));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to modify Command.def as well, to change the description of the command (which I recommend below to apply to the complete command, instead of candidates).

break;
case Command::CompletionDiagnostics:
case Command::Diagnostics:
Expand Down
1 change: 1 addition & 0 deletions server/src/Command.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ struct Command {
std::string unsavedFile;
std::string dir;
std::string prefix;
std::string sorting;
PrefixMatchStyle style;
unsigned line;
unsigned column;
Expand Down
55 changes: 43 additions & 12 deletions server/src/Irony.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,23 @@
#include <iostream>
#include <fstream>
#include <cctype>
#include <sstream>
#include <vector>

namespace {

struct CompletionCandidate
{
unsigned int priority;
std::string str;

CompletionCandidate(unsigned int priority, const std::string& str) : priority(priority), str(str) {}

bool operator < (const CompletionCandidate& candidate) const {
return (priority < candidate.priority);
}
};

std::string cxStringToStd(CXString cxString) {
std::string stdStr;

Expand Down Expand Up @@ -359,7 +373,7 @@ void Irony::completionDiagnostics() const {
std::cout << ")\n";
}

void Irony::candidates(const std::string &prefix, PrefixMatchStyle style) const {
void Irony::candidates(const std::string &prefix, PrefixMatchStyle style, const std::string &sorting) const {
if (activeCompletionResults_ == nullptr) {
std::cout << "nil\n";
return;
Expand All @@ -375,6 +389,7 @@ void Irony::candidates(const std::string &prefix, PrefixMatchStyle style) const
std::string typedtext, brief, resultType, prototype, postCompCar, available;

std::vector<unsigned> postCompCdr;
std::vector<CompletionCandidate> candidates;

for (unsigned i = 0; i < completions->NumResults; ++i) {
CXCompletionResult candidate = completions->Results[i];
Expand Down Expand Up @@ -508,19 +523,35 @@ void Irony::candidates(const std::string &prefix, PrefixMatchStyle style) const
clang_getCompletionBriefComment(candidate.CompletionString));
#endif

std::stringstream candidateSS;

// see irony-completion.el#irony-completion-candidates
std::cout << '(' << support::quoted(typedtext)
<< ' ' << priority
<< ' ' << support::quoted(resultType)
<< ' ' << support::quoted(brief)
<< ' ' << support::quoted(prototype)
<< ' ' << annotationStart
<< " (" << support::quoted(postCompCar);
candidateSS << '(' << support::quoted(typedtext)
<< ' ' << priority
<< ' ' << support::quoted(resultType)
<< ' ' << support::quoted(brief)
<< ' ' << support::quoted(prototype)
<< ' ' << annotationStart
<< " (" << support::quoted(postCompCar);
for (unsigned index : postCompCdr)
std::cout << ' ' << index;
std::cout << ")"
<< ' ' << available
<< ")\n";
candidateSS << ' ' << index;
candidateSS << ")"
<< ' ' << available
<< ")\n";

// Add candidate to candidate list
candidates.push_back(CompletionCandidate(priority, candidateSS.str()));
}

if (sorting == "alphabetical-clang") {
// Sort candidates list
// ASC - Smaller values indicate higher-priority (more likely) completions.
std::sort(candidates.begin(), candidates.end());
}

// Output sorted list
for (auto &candidate : candidates) {
std::cout << candidate.str;
}

std::cout << ")\n";
Expand Down
2 changes: 1 addition & 1 deletion server/src/Irony.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ class Irony {
/// Get all the completion candidates.
///
/// \pre complete() was called.
void candidates(const std::string &prefix, PrefixMatchStyle style) const;
void candidates(const std::string &prefix, PrefixMatchStyle style, const std::string &sorting) const;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do like PrefixMatchStyle, and make a dedicated enum to represent the possibles values instead of storing it as a string.


/// Get the diagnostics produced by the last \c complete().
///
Expand Down
2 changes: 1 addition & 1 deletion server/src/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ int main(int ac, const char *av[]) {
break;

case Command::Candidates:
irony.candidates(c->prefix, c->style);
irony.candidates(c->prefix, c->style, c->sorting);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could make more sense to do this in the complete command, instead of candidates.
complete is called once but candidates many time.
Since the sorting does not depend on the candidates input, it may be better to do the sorting once, next to the sorting done by clang.

break;

case Command::CompletionDiagnostics:
Expand Down