Fix override function signature of createCompletionContext()
ClosedPublic

Authored by myang on Dec 16 2018, 5:07 AM.

Diff Detail

Repository
R866 KDevelop Rust Support
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
myang created this revision.Dec 16 2018, 5:07 AM
myang created this object with edit policy "Administrators".
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptDec 16 2018, 5:07 AM
myang requested review of this revision.Dec 16 2018, 5:07 AM
mwolff added a subscriber: mwolff.Dec 17 2018, 8:41 AM

Please rephrase your commit message to indicate what you are fixing. This seems to be a change that fixes a minor performance paper cut, right?

myang added a comment.Dec 17 2018, 9:07 AM

@mwolff No, it's not to improve performance. This project will not be built without these changes.
createCompletionContext is an override function, so its definition must correspond with the original one from /usr/include/kdevplatform/language/codecompletion/codecompletionworker.h.

Referrence:
https://github.com/KDE/kdevelop/blob/master/kdevplatform/language/codecompletion/codecompletionworker.h#L82
https://github.com/KDE/kdevelop/blob/master/kdevplatform/language/codecompletion/codecompletionworker.cpp#L141

myang retitled this revision from Fix createCompletionContext to Fix definition of createCompletionContext().Dec 17 2018, 9:16 AM
mwolff requested changes to this revision.Dec 17 2018, 9:19 AM

See, that's why the commit message should be rephrased :)

Also, add the missing "override" keyword to make it clear that this overrides a function (and thus needs to match the signature)

This revision now requires changes to proceed.Dec 17 2018, 9:19 AM
myang retitled this revision from Fix definition of createCompletionContext() to Fix override function signature of createCompletionContext().Dec 17 2018, 10:12 AM
myang added a comment.Dec 22 2018, 2:56 AM

@mwolff Still "Needs Revision", is the commit message not accurate enough?

Wouldn't that be an opportunity to add the override specifier? The commit message could state "Fix function signature of ... to override".

mwolff accepted this revision.Jan 28 2019, 10:10 AM

lgtm now, sorry for the delay - can you push that yourself? if not, please tell us your email address and name to associate with the commit

thanks

This revision is now accepted and ready to land.Jan 28 2019, 10:10 AM
This revision was automatically updated to reflect the committed changes.