-
Notifications
You must be signed in to change notification settings - Fork 766
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
[READY] Remove boost where only simple refactoring is needed #701
Conversation
Removes every boost::algorithm, boost::exceptions and boost::assign. Removes standard.h in favour of using size_t.
Codecov Report
@@ Coverage Diff @@
## master #701 +/- ##
==========================================
+ Coverage 92.68% 92.75% +0.06%
==========================================
Files 79 79
Lines 5265 5284 +19
Branches 153 156 +3
==========================================
+ Hits 4880 4901 +21
+ Misses 325 323 -2
Partials 60 60 Continue to review full report at Codecov.
|
Travis is passing, but appveyor is stalling on MSVC12. Should we move away from Also, |
Also the errors on MSVC11 are caused by replacing boost::assign. But the syntax I used is the same that is valid in C when one needs to assign multidimensional array of characters aand is valid in C99. |
I think we have to check which version of the Reviewed 16 of 21 files at r1. cpp/ycm/Candidate.cpp, line 34 at r1 (raw file):
Didn't we loose the locale here? Shouldn't we use this version of isprint? So maybe instead of a lambda we could do: namespace detail
{
const locale& loc = std::locale::classic();
bool isprint( char c ) {
return std::locale::isprint( c, loc );
}
}
...
return all_of( text.begin(), text.end(), detail::isprint ); Or if we want to keep the lambda just use cpp/ycm/Candidate.cpp, line 81 at r1 (raw file):
If we go changin the lambda above we could make a proxy function for cpp/ycm/IdentifierDatabase.cpp, line 89 at r1 (raw file):
I think this is longer than 80 column. cpp/ycm/ClangCompleter/Location.h, line 35 at r1 (raw file):
this is now longer than 80 columns cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 25 at r1 (raw file):
I think I'm missing something: what are those header for? Comments from Reviewable |
Yes.
MSVC does not support C99 yet; I think they're working on improving that in future versions.
Sounds like a plan! Needs to work though. :) Review status: 16 of 21 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. Comments from Reviewable |
Review status: 16 of 21 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed. cpp/ycm/exceptions.h, line 34 at r1 (raw file):
Hm, not sure is this a safe change. Then again, was never a huge fan of boost::exception so even if we lose some of its functionality, I don't care too much. cpp/ycm/Result.cpp, line 222 at r1 (raw file):
You probably want to take params by const ref, not by value. The latter introduces copies. So: cpp/ycm/Result.h, line 52 at r1 (raw file):
const ref (see previous comment) cpp/ycm/ClangCompleter/CompletionData.cpp, line 149 at r1 (raw file):
Const ref params. cpp/ycm/ClangCompleter/CompletionData.cpp, line 160 at r1 (raw file):
This does not have the same perf. The old version would mutate cpp/ycm/ClangCompleter/CompletionData.h, line 123 at r1 (raw file):
Const ref params. Comments from Reviewable |
Pass strings by reference. Don't copy a string when truncating. Pay attention to 80 column width.
I'll get on with MSVC -std=c++11 flag will be fixed in the next push.
The error regarding removal of I figured out why that one test is failing if we switch from
Since there are changeable numbers, I believe it would be best to match the whole line with a regex. Review status: 15 of 22 files reviewed at latest revision, 12 unresolved discussions. cpp/ycm/Candidate.cpp, line 34 at r1 (raw file): Previously, vheon (Andrea Cedraro) wrote…
I like how readable the cpp/ycm/Candidate.cpp, line 81 at r1 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Agreed with going with the namespace option. cpp/ycm/exceptions.h, line 34 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
I was thinking the same, but it's passing all the tests, so we can leave it for now and we can always write more tests later. cpp/ycm/IdentifierDatabase.cpp, line 89 at r1 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Will be fixed after introducing cpp/ycm/Result.cpp, line 222 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. cpp/ycm/Result.h, line 52 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. cpp/ycm/ClangCompleter/CompletionData.cpp, line 149 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. cpp/ycm/ClangCompleter/CompletionData.cpp, line 160 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. cpp/ycm/ClangCompleter/CompletionData.h, line 123 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. cpp/ycm/ClangCompleter/Location.h, line 35 at r1 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Done. cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 25 at r1 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Comments from Reviewable |
Can anyone explain why would compiling libracerd on travis on linux fail? |
Could be the new version of Rust (1.15.0 was just released). PR #699 may fix that. Review status: 15 of 22 files reviewed at latest revision, 11 unresolved discussions. Comments from Reviewable |
The builds and tests on CI succeeded. Is there anything else that needs to be done before pushing Review status: 12 of 23 files reviewed at latest revision, 6 unresolved discussions. cpp/ycm/Result.cpp, line 225 at r4 (raw file):
Am I missing something or will this never happen? As far as I can see That would mean this check is redundant and provides nothing except one more line impossible to cover with tests. Comments from Reviewable |
Review status: 12 of 23 files reviewed at latest revision, 6 unresolved discussions. cpp/ycm/Candidate.cpp, line 81 at r1 (raw file): Previously, bstaletic wrote…
Yeah I would very much prefer an extracted function than passing a lambda. cpp/ycm/Result.cpp, line 225 at r4 (raw file): Previously, bstaletic wrote…
If you're confident it can't be hit, remove it. :) Comments from Reviewable |
Review status: 12 of 25 files reviewed at latest revision, 6 unresolved discussions. cpp/ycm/Candidate.cpp, line 81 at r1 (raw file): Previously, Valloric (Val Markovic) wrote…
Done. cpp/ycm/Result.cpp, line 225 at r4 (raw file): Previously, Valloric (Val Markovic) wrote…
Still looking into it. Identifier match ranking algorithm is really complex. (: Comments from Reviewable |
I pushed a commit that replaces The only place where Feel free to point out where have I gone wrong. Review status: 11 of 34 files reviewed at latest revision, 6 unresolved discussions. Comments from Reviewable |
Review status: 11 of 34 files reviewed at latest revision, 6 unresolved discussions. cpp/ycm/Result.cpp, line 225 at r4 (raw file): Previously, bstaletic wrote…
Now I am confident. That function will only be called if query is a subsequence of text. Comments from Reviewable |
To answer this, it's necessary when you change an external-facing API in the C++ code. Such APIs are the ones used by the YCM Python code. You probably need to bump it now. With that, I think I'm on this PR now! :) Review status: 11 of 34 files reviewed at latest revision, 6 unresolved discussions. Comments from Reviewable |
I have to say I'm not a super fan of an entire Translation Unit just for 3 function 2 of which are used in only one location and the other one in 2. I mean at this point for static bool isprint( char c ) {
return std::locale::isprint( c, std::locale::classic() );
}
static bool islower( char c ) {
return std::locale::islower( c, std::locale::classic() );
} would suffice (avoiding a new TU and then the linking). I don't have the strongest opinion on this so is not a deal breaker to merge this for me. Reviewed 1 of 21 files at r1, 3 of 6 files at r2, 2 of 2 files at r4, 14 of 16 files at r5, 1 of 2 files at r6. cpp/ycm/Detail.h, line 18 at r5 (raw file):
I don't know if could be a problem but for the rest of the files the defined symbol is postfixed with a random string (I guess Val [used this snippet] (https://github.com/honza/vim-snippets/blob/master/UltiSnips/cpp.snippets#L57-L72) for the other files) to avoid conflict. I don't know how common is cpp/ycm/Detail.h, line 27 at r5 (raw file):
Looks like cpp/ycm/IdentifierDatabase.cpp, line 87 at r5 (raw file):
I think we have tabs here while re rest looks like is indented with spaces. cpp/ycm/PythonSupport.cpp, line 85 at r5 (raw file):
Also here looks like there is a mixed of tabs and spaces. cpp/ycm/ycm_core.cpp, line 225 at r5 (raw file):
Aren't we still using boost thread? why is safe to remove this? Comments from Reviewable |
@vheon Review status: 28 of 33 files reviewed at latest revision, 7 unresolved discussions. cpp/ycm/IdentifierDatabase.cpp, line 87 at r5 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Switched to static functions to not introduce a new translation unit. cpp/ycm/PythonSupport.cpp, line 85 at r5 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Switched to static functions to not introduce a new translation unit. cpp/ycm/ycm_core.cpp, line 225 at r5 (raw file): Previously, vheon (Andrea Cedraro) wrote…
The PR #697 made a switch from boost thread to C++11 threads. cpp/ycm/Detail.h, line 18 at r5 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Switched to static functions to not introduce a new translation unit. cpp/ycm/Detail.h, line 27 at r5 (raw file): Previously, vheon (Andrea Cedraro) wrote…
Done. Comments from Reviewable |
Reviewed 1 of 6 files at r2, 1 of 16 files at r5, 4 of 5 files at r7. cpp/ycm/ycm_core.cpp, line 225 at r5 (raw file): Previously, bstaletic wrote…
I read Comments from Reviewable |
What's up with codecov again? Also, the last change provides no reason for coverage to change at all. |
Reviewed 13 of 21 files at r1, 5 of 6 files at r2, 3 of 3 files at r3, 2 of 2 files at r4, 11 of 16 files at r5, 1 of 2 files at r6, 3 of 5 files at r7. cpp/ycm/PythonSupport.cpp, line 62 at r7 (raw file):
The same function is defined in cpp/ycm/Result.cpp, line 229 at r7 (raw file):
Missing blank line. cpp/ycm/ClangCompleter/CompletionData.cpp, line 156 at r4 (raw file):
Missing two blank lines. Comments from Reviewable |
Review status: 29 of 35 files reviewed at latest revision, 6 unresolved discussions. cpp/ycm/PythonSupport.cpp, line 62 at r7 (raw file): Previously, micbou wrote…
Done. cpp/ycm/Result.cpp, line 229 at r7 (raw file): Previously, micbou wrote…
Done. cpp/ycm/ClangCompleter/CompletionData.cpp, line 156 at r4 (raw file): Previously, micbou wrote…
Done. Comments from Reviewable |
Reviewed 6 of 6 files at r8. Comments from Reviewable |
Reviewed 6 of 6 files at r8. cpp/ycm/Utils.cpp, line 56 at r8 (raw file):
Opening curly brace should be put on the same line. Comments from Reviewable |
Review status: 34 of 35 files reviewed at latest revision, 3 unresolved discussions. cpp/ycm/Utils.cpp, line 56 at r8 (raw file): Previously, micbou wrote…
Done. Comments from Reviewable |
I see 2 LGTM's and open issues resolved, so here we go! @bstaletic Thanks a ton for the PR! @homu r+ Review status: 34 of 35 files reviewed at latest revision, 2 unresolved discussions. Comments from Reviewable |
📌 Commit 3c2f47a has been approved by |
[READY] Remove boost where only simple refactoring is needed This pull request continues with removing boost components with std counterparts. Only simple refactoring is supposed to be done here. Current changes: - Removes boost/algorithm - Removes boost/exceptions - Removes boost/assign - Removes standard.h in favour of using `size_t`, or `unsigned int` - Minimises boost/noncopyable, leaving it only in one file #### Things on a TODO list: ~~Write tests if coverage complains~~ - [x] Bump ycmd core version (Is this needed?) <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/701) <!-- Reviewable:end -->
@Valloric No problem. But this is not over yet. So you'll be seeing more of me soon. :D |
💔 Test failed - status |
@homu retry Reviewed 1 of 1 files at r9. Comments from Reviewable |
[READY] Remove boost where only simple refactoring is needed This pull request continues with removing boost components with std counterparts. Only simple refactoring is supposed to be done here. Current changes: - Removes boost/algorithm - Removes boost/exceptions - Removes boost/assign - Removes standard.h in favour of using `size_t`, or `unsigned int` - Minimises boost/noncopyable, leaving it only in one file #### Things on a TODO list: ~~Write tests if coverage complains~~ - [x] Bump ycmd core version (Is this needed?) <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/701) <!-- Reviewable:end -->
☀️ Test successful - status |
[READY] Add MSVC 15 support [Visual Studio 2017 (MSVC 15) was released this week](https://www.visualstudio.com/en-us/news/releasenotes/vs2017-relnotes) and AppVeyor just added support for it. MSVC 11 support was dropped in PR #701 so we remove it from our build script. Closes #656. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/717) <!-- Reviewable:end -->
[READY] Switch from boost to std for hashes, arrays and regexes This is a continuation of #701. These changes will not be trivial and will need to be discussed before proceeding. At this moment only `std::hash` is working. The other two boost components will be discussed after agreeing that this hash implementation is fine. - [x] Use `std::hash` instead of `boost::hash` - [x] Use `std::array` instead of `boost::array` ~~Use `regex_search` and related from `std` namespace.~~ ### Regex component will be in another PR As it stands for now the regex part is the hardest to replce, so, in order not to make this PR big and drag it out, regex component will be in a separate PR. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/705) <!-- Reviewable:end -->
I was merging upstream with my fork and noticed some strange additions in this pull request, for example, does it make sense that the Comments from Reviewable |
Remove IdentifierEndsWith() declaration Just like @oblitum pointed out in [#701(comment)](#701 (comment)), in `CompletionData.cpp` the `IdentifierEndsWith()` function is called only from function `RemoveTrailingParens()` and both functions are in unnamed namespace, so neither should have declaration in `CompletionData.h`. @oblitum, in his comment, mentions that he had noticed more odd changes in that PR. While this commit fixes the change he explicitely mentioned I would like to wait and see if he has to add anything else. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/732) <!-- Reviewable:end -->
This pull request continues with removing boost components with std counterparts. Only simple refactoring is supposed to be done here. Current changes:
size_t
, orunsigned int
Things on a TODO list:
Write tests if coverage complainsThis change is