[READY] Remove boost where only simple refactoring is needed #701

bstaletic · 2017-02-02T10:40:22Z

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~~

Bump ycmd core version (Is this needed?)

This change is

Removes every boost::algorithm, boost::exceptions and boost::assign. Removes standard.h in favour of using size_t.

codecov-io · 2017-02-02T11:24:12Z

Codecov Report

Merging #701 into master will increase coverage by 0.06%.
The diff coverage is 97.02%.

@@            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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec7a154...3c2f47a. Read the comment docs.

bstaletic · 2017-02-02T12:12:35Z

Travis is passing, but appveyor is stalling on MSVC12.

Should we move away from size_t in order to silence appveyor warnings?

Also, -std=c++11 on MSVC is an unknown option, which is rightfully ignored, but MSVC emits a warning. Should we fix that too?

bstaletic · 2017-02-02T12:20:26Z

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.

vheon · 2017-02-02T21:45:00Z

I think we have to check which version of the islower isupper we should use, because in this change we're using the one from ctypes while there are counterparts in locale. For the use of isprint was clear that we were missing the locale but what about the other ones?

Reviewed 16 of 21 files at r1.
Review status: 16 of 21 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

cpp/ycm/Candidate.cpp, line 34 at r1 (raw file):

  return all_of( text.cbegin(),
                 text.cend(),
                 []( char c ) { return isprint( c ); } );

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 std::locale::isprint( c, std::locale::classic() ).

cpp/ycm/Candidate.cpp, line 81 at r1 (raw file):

  text_is_lowercase_( all_of( text.cbegin(),
                              text.cend(),
                              []( char c ) { return islower( c ); } ) ),

If we go changin the lambda above we could make a proxy function for islower in the detail namespace as well so this could be a one-liner that is easy to read.

cpp/ycm/IdentifierDatabase.cpp, line 89 at r1 (raw file):

  bool query_has_uppercase_letters = any_of( query.cbegin(),
                                             query.cend(),
                                             []( char c ) { return isupper( c ); } );

I think this is longer than 80 column.

cpp/ycm/ClangCompleter/Location.h, line 35 at r1 (raw file):

      filename_( "" ) {}

  Location( const std::string &filename, unsigned int line, unsigned int column )

this is now longer than 80 columns

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 25 at r1 (raw file):

#include <cstdlib>
#include <algorithm>

I think I'm missing something: what are those header for?

Comments from Reviewable

Valloric · 2017-02-02T22:28:10Z

Also, -std=c++11 on MSVC is an unknown option, which is rightfully ignored, but MSVC emits a warning. Should we fix that too?

Yes.

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.

MSVC does not support C99 yet; I think they're working on improving that in future versions.

replicating that header in our own NonCopyable.h is as simple as making a class with only constructor, destructor and copy constructor.

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

Valloric · 2017-02-02T22:33:35Z

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):

 * The common base for all exceptions.
 */
struct ExceptionBase: virtual std::exception, virtual boost::exception {};

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):

bool Result::QueryIsPrefix( std::string text, std::string query ) {

You probably want to take params by const ref, not by value. The latter introduces copies.

So: const std::string& text and same for the other param.

cpp/ycm/Result.h, line 52 at r1 (raw file):

    const std::string &word_boundary_chars );

  bool QueryIsPrefix( std::string text, std::string query );

const ref (see previous comment)

cpp/ycm/ClangCompleter/CompletionData.cpp, line 149 at r1 (raw file):

bool IdentifierEndsWith( std::string identifier, std::string end ) {

Const ref params.

cpp/ycm/ClangCompleter/CompletionData.cpp, line 160 at r1 (raw file):

std::string RemoveTrailingParens( std::string text ) {
  if ( IdentifierEndsWith( text, "(" ) ) {
    text = text.substr( 0, text.length() - 1 );

This does not have the same perf. The old version would mutate text, the new version makes a copy.

cpp/ycm/ClangCompleter/CompletionData.h, line 123 at r1 (raw file):

                             bool &saw_placeholder );

  bool IdentifierEndsWith( std::string identifier, std::string end );

Const ref params.

Comments from Reviewable

Pass strings by reference. Don't copy a string when truncating. Pay attention to 80 column width.

bstaletic · 2017-02-03T00:01:29Z

Sounds like a plan! Needs to work though. :)

I'll get on with utility.hpp replacing when everything else is done.

MSVC -std=c++11 flag will be fixed in the next push.

MSVC does not support C99 yet; I think they're working on improving that in future versions.

The error regarding removal of boost::assign happens only on MSVC11. Do we simply drop the support for it?

I figured out why that one test is failing if we switch from boost::array to std::array.
It is not that it doesn't throw std::out_of_range, it's just that its error.what() is rather cryptic with the following format:

array::at: __n (which is <very long number that can change depending on the actual index>) >= _Nm (which is <another changable numbe which is in ycmd defined as 256>)

Since there are changeable numbers, I believe it would be best to match the whole line with a regex.
Would the following regex work: array::at: __n -?\(which is \\d+\) >= _Nm \(which is \\d+\)

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…

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 std::locale::isprint( c, std::locale::classic() ).

I like how readable the namespace detail variant is. So I'd like to go with that.
Would difining that in cpp/ycm/Detail.h and implementing in cpp/ycm/Detail.cpp be okay?

cpp/ycm/Candidate.cpp, line 81 at r1 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

If we go changin the lambda above we could make a proxy function for islower in the detail namespace as well so this could be a one-liner that is easy to read.

Agreed with going with the namespace option.

cpp/ycm/exceptions.h, line 34 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

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.

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…

I think this is longer than 80 column.

Will be fixed after introducing isupper from the detail namespace, which is discussed above.

cpp/ycm/Result.cpp, line 222 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

You probably want to take params by const ref, not by value. The latter introduces copies.

So: const std::string& text and same for the other param.

Done.

cpp/ycm/Result.h, line 52 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

const ref (see previous comment)

Done.

cpp/ycm/ClangCompleter/CompletionData.cpp, line 149 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Const ref params.

Done.

cpp/ycm/ClangCompleter/CompletionData.cpp, line 160 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

This does not have the same perf. The old version would mutate text, the new version makes a copy.

Done.

cpp/ycm/ClangCompleter/CompletionData.h, line 123 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

Const ref params.

Done.

cpp/ycm/ClangCompleter/Location.h, line 35 at r1 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

this is now longer than 80 columns

Done.

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 25 at r1 (raw file):

Previously, vheon (Andrea Cedraro) wrote…

I think I'm missing something: what are those header for?

algorith is needed for std::all_of and std::any_of.
cstdlib is needed for std::abs. Travis on OSX complained about this one.

Comments from Reviewable

bstaletic · 2017-02-03T00:57:57Z

Can anyone explain why would compiling libracerd on travis on linux fail?

micbou · 2017-02-03T01:10:25Z

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

bstaletic · 2017-02-04T10:48:27Z

The builds and tests on CI succeeded. Is there anything else that needs to be done before pushing boost::array to std::array change?

Review status: 12 of 23 files reviewed at latest revision, 6 unresolved discussions.

cpp/ycm/Result.cpp, line 225 at r4 (raw file):

                            const std::string &query ) {
  if ( text.length() < query.length() )
    return false;

Am I missing something or will this never happen?

As far as I can see SetResultFeaturesFromQuery() won't be called if query doesn't hit anything stored in identifier database. As a result QueryIsPrefix() also won't be called.

That would mean this check is redundant and provides nothing except one more line impossible to cover with tests.

Comments from Reviewable

Valloric · 2017-02-04T19:20:51Z

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…

Agreed with going with the namespace option.

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…

Am I missing something or will this never happen?

As far as I can see SetResultFeaturesFromQuery() won't be called if query doesn't hit anything stored in identifier database. As a result QueryIsPrefix() also won't be called.

That would mean this check is redundant and provides nothing except one more line impossible to cover with tests.

If you're confident it can't be hit, remove it. :)

Comments from Reviewable

bstaletic · 2017-02-04T20:42:12Z

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…

Yeah I would very much prefer an extracted function than passing a lambda.

Done.

cpp/ycm/Result.cpp, line 225 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

If you're confident it can't be hit, remove it. :)

Still looking into it. Identifier match ranking algorithm is really complex. (:
The actual method I'm looking at, which will eventually answer this question, is Result Candidate::QueryMatchResult( const std::string& text) implemented in Candidate.cpp on line 80.

Comments from Reviewable

bstaletic · 2017-02-05T15:58:18Z

I pushed a commit that replaces boost::noncopyable with the actual code that deletes the copy construct.

The only place where boost::noncopyable is left is in cpp/ycm/ycm_core.cpp when exposing classes to python.
If I'm not missing anything big, boost::python::class_ depends on boost::noncopyable to realise that the class is noncopyable and not to try to access the copy construct.

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

bstaletic · 2017-02-06T11:16:40Z

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…

Still looking into it. Identifier match ranking algorithm is really complex. (:
The actual method I'm looking at, which will eventually answer this question, is Result Candidate::QueryMatchResult( const std::string& text) implemented in Candidate.cpp on line 80.

Now I am confident. That function will only be called if query is a subsequence of text.
"fooooo" is most definitely not a subsequence of "foo", so the check is definitely redundant.

Comments from Reviewable

Valloric · 2017-02-06T17:59:12Z

Bump ycmd core version (Is this needed?)

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

vheon · 2017-02-06T18:55:42Z

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 isprint and islower a static function like:

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.
Review status: 32 of 35 files reviewed at latest revision, 7 unresolved discussions.

cpp/ycm/Detail.h, line 18 at r5 (raw file):

// along with ycmd.  If not, see <http://www.gnu.org/licenses/>.

#ifndef DETAIL_H

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 DETAIL_H though 😕

cpp/ycm/Detail.h, line 27 at r5 (raw file):

namespace detail {

const std::locale loc( "C" );

Looks like std::locale::classic() is actually a static variable to this exact locale so I guess we can just use that (ref. http://en.cppreference.com/w/cpp/locale/locale/classic)

cpp/ycm/IdentifierDatabase.cpp, line 87 at r5 (raw file):

  bool query_has_uppercase_letters = any_of( query.cbegin(),
                                             query.cend(),
					     detail::isupper );

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):

    bool query_has_uppercase_letters = any_of( query.cbegin(),
                                               query.cend(),
					       detail::isupper );

Also here looks like there is a mixed of tabs and spaces.

cpp/ycm/ycm_core.cpp, line 225 at r5 (raw file):

// Boost.Thread forces us to implement this.
// We don't use any thread-specific (local) storage so it's fine to implement

Aren't we still using boost thread? why is safe to remove this?

Comments from Reviewable

bstaletic · 2017-02-06T19:22:21Z

@vheon
I removed Detail.h and Detail.cpp in favour of static functions.

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…

I think we have tabs here while re rest looks like is indented with spaces.

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…

Also here looks like there is a mixed of tabs and spaces.

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…

Aren't we still using boost thread? why is safe to remove this?

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…

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 DETAIL_H though 😕

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…

Looks like std::locale::classic() is actually a static variable to this exact locale so I guess we can just use that (ref. http://en.cppreference.com/w/cpp/locale/locale/classic)

Done.

Comments from Reviewable

vheon · 2017-02-06T20:48:37Z

Reviewed 1 of 6 files at r2, 1 of 16 files at r5, 4 of 5 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

cpp/ycm/ycm_core.cpp, line 225 at r5 (raw file):

Previously, bstaletic wrote…

The PR #697 made a switch from boost thread to C++11 threads.

I read thread and actually thought about boost::thread while we only use the locks here and the threads are in Python-land (at least I think otherwise I really don't know what I'm talking about 😒 )

Comments from Reviewable

bstaletic · 2017-02-06T22:55:41Z

What's up with codecov again?
In some places it is reporting an increase in coverage by 0.06%.
In others it is reporting a drop in coverage even in places I didn't touch.

Also, the last change provides no reason for coverage to change at all.

micbou · 2017-02-06T23:03:07Z

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.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

cpp/ycm/PythonSupport.cpp, line 62 at r7 (raw file):

static bool IsUpper( char c ) {
  return std::isupper( c, std::locale::classic() );
}

The same function is defined in IdentifierDatabase.cpp. I think we should move it to Utils.h and include this header (it's already included in IdentifierDatabase.cpp).

cpp/ycm/Result.cpp, line 229 at r7 (raw file):

  return true;
}

Missing blank line.

cpp/ycm/ClangCompleter/CompletionData.cpp, line 156 at r4 (raw file):

                                    end );
  return false;
}

Missing two blank lines.

Comments from Reviewable

bstaletic · 2017-02-06T23:37:37Z

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…

The same function is defined in IdentifierDatabase.cpp. I think we should move it to Utils.h and include this header (it's already included in IdentifierDatabase.cpp).

Done.

cpp/ycm/Result.cpp, line 229 at r7 (raw file):

Previously, micbou wrote…

Missing blank line.

Done.

cpp/ycm/ClangCompleter/CompletionData.cpp, line 156 at r4 (raw file):

Previously, micbou wrote…

Missing two blank lines.

Done.

Comments from Reviewable

vheon · 2017-02-07T21:20:47Z

Reviewed 6 of 6 files at r8.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

micbou · 2017-02-13T15:30:36Z

Reviewed 6 of 6 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

cpp/ycm/Utils.cpp, line 56 at r8 (raw file):

bool IsUpper( char c )
{

Opening curly brace should be put on the same line.

Comments from Reviewable

bstaletic · 2017-02-13T16:56:07Z

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…

Opening curly brace should be put on the same line.

Done.

Comments from Reviewable

Valloric · 2017-02-13T18:37:31Z

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

homu · 2017-02-13T18:37:33Z

📌 Commit 3c2f47a has been approved by Valloric

[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?)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/701)

homu · 2017-02-13T18:37:47Z

⌛ Testing commit 3c2f47a with merge 6848e07...

bstaletic · 2017-02-13T18:59:25Z

@Valloric No problem. But this is not over yet. So you'll be seeing more of me soon. :D

homu · 2017-02-13T20:57:37Z

💔 Test failed - status

micbou · 2017-02-13T22:11:24Z

@homu retry

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

homu · 2017-02-13T22:11:34Z

⌛ Testing commit 3c2f47a with merge c36ccb9...

[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?)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/701)

homu · 2017-02-14T00:45:22Z

☀️ 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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/717)

[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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/705)

oblitum · 2017-03-12T04:06:03Z

I was merging upstream with my fork and noticed some strange additions in this pull request, for example, does it make sense that the IdentifierEndsWith function is declared as a private member of CompletionData but is defined as a free function in an anonymous namespace?

Comments from Reviewable

@oblitum

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.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/732)

bstaletic added 2 commits February 2, 2017 10:37

Further reduce boost usage

3b0b04a

Removes every boost::algorithm, boost::exceptions and boost::assign. Removes standard.h in favour of using size_t.

Fix error on OSX

d0e5dde

bstaletic added 2 commits February 3, 2017 00:55

Style fixes and performance optimisations

298733b

Pass strings by reference. Don't copy a string when truncating. Pay attention to 80 column width.

Don't set -std=c++11 on MSVC

cdea614

bstaletic added 2 commits February 3, 2017 23:49

Use isprint() and similar with locale support

c7be68d

Drop MSVC 11 to pass appveyor ci

0661215

bstaletic added 3 commits February 4, 2017 22:09

Add a wrapper around isprint(), islower() and isprint()

5583da3

Boost.Thread not used any more

bd86ed2

Remove boost::noncopyable where possible

43fac50

Remove redundant check that won't ever be reached

9cf8275

core_version bump and tabs to spaces

434880f

bstaletic changed the title ~~[WIP] Remove boost where only simple refactoring is needed~~ [READY] Remove boost where only simple refactoring is needed Feb 6, 2017

Don't introuce new translation unit for isupper()

38ad0bb

Include <locale> header

133fa7e

Move IsUpper to Utils.h

ead191d

Opening curly brace on the same line

3c2f47a

homu merged commit 3c2f47a into ycm-core:master Feb 14, 2017

homu mentioned this pull request Feb 14, 2017

[Ready] Swift Programming Language Completions #487

Closed

bstaletic mentioned this pull request Feb 14, 2017

[READY] Switch from boost to std for hashes, arrays and regexes #705

Merged

2 tasks

micbou mentioned this pull request Mar 10, 2017

[READY] Add MSVC 15 support #717

Merged

bstaletic mentioned this pull request Mar 30, 2017

Remove IdentifierEndsWith() declaration #732

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Remove boost where only simple refactoring is needed #701

[READY] Remove boost where only simple refactoring is needed #701

bstaletic commented Feb 2, 2017 •

edited

Loading

codecov-io commented Feb 2, 2017 •

edited

Loading

bstaletic commented Feb 2, 2017

bstaletic commented Feb 2, 2017

vheon commented Feb 2, 2017

Valloric commented Feb 2, 2017

Valloric commented Feb 2, 2017

bstaletic commented Feb 3, 2017

bstaletic commented Feb 3, 2017

micbou commented Feb 3, 2017

bstaletic commented Feb 4, 2017

Valloric commented Feb 4, 2017

bstaletic commented Feb 4, 2017

bstaletic commented Feb 5, 2017

bstaletic commented Feb 6, 2017

Valloric commented Feb 6, 2017

vheon commented Feb 6, 2017

bstaletic commented Feb 6, 2017

vheon commented Feb 6, 2017

bstaletic commented Feb 6, 2017

micbou commented Feb 6, 2017

bstaletic commented Feb 6, 2017

vheon commented Feb 7, 2017

micbou commented Feb 13, 2017

bstaletic commented Feb 13, 2017

Valloric commented Feb 13, 2017

homu commented Feb 13, 2017

homu commented Feb 13, 2017

bstaletic commented Feb 13, 2017

homu commented Feb 13, 2017

micbou commented Feb 13, 2017

homu commented Feb 13, 2017

homu commented Feb 14, 2017

oblitum commented Mar 12, 2017

[READY] Remove boost where only simple refactoring is needed #701

[READY] Remove boost where only simple refactoring is needed #701

Conversation

bstaletic commented Feb 2, 2017 • edited Loading

Things on a TODO list:

codecov-io commented Feb 2, 2017 • edited Loading

Codecov Report

bstaletic commented Feb 2, 2017

bstaletic commented Feb 2, 2017

vheon commented Feb 2, 2017

Valloric commented Feb 2, 2017

Valloric commented Feb 2, 2017

bstaletic commented Feb 3, 2017

bstaletic commented Feb 3, 2017

micbou commented Feb 3, 2017

bstaletic commented Feb 4, 2017

Valloric commented Feb 4, 2017

bstaletic commented Feb 4, 2017

bstaletic commented Feb 5, 2017

bstaletic commented Feb 6, 2017

Valloric commented Feb 6, 2017

vheon commented Feb 6, 2017

bstaletic commented Feb 6, 2017

vheon commented Feb 6, 2017

bstaletic commented Feb 6, 2017

micbou commented Feb 6, 2017

bstaletic commented Feb 6, 2017

vheon commented Feb 7, 2017

micbou commented Feb 13, 2017

bstaletic commented Feb 13, 2017

Valloric commented Feb 13, 2017

homu commented Feb 13, 2017

homu commented Feb 13, 2017

bstaletic commented Feb 13, 2017

homu commented Feb 13, 2017

micbou commented Feb 13, 2017

homu commented Feb 13, 2017

homu commented Feb 14, 2017

oblitum commented Mar 12, 2017

bstaletic commented Feb 2, 2017 •

edited

Loading

codecov-io commented Feb 2, 2017 •

edited

Loading