[READY] Translate libclang error codes to exceptions

[micbou]

This PR translates libclang error codes returned by the clang_parseTranslationUnit2FullArgv and clang_reparseTranslationUnit functions to std::runtime_error exceptions with the corresponding error message; replacing the ClangParseError exception. Unlike ClangParseError, we don't silence them as they can help understand why the Clang completer is not working.

---

This change is 

[Valloric]

Thanks for the PR!

---

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #861 into master will increase coverage by 0.09%.
The diff coverage is 94.23%.

@@            Coverage Diff             @@
##           master     #861      +/-   ##
==========================================
+ Coverage    96.2%   96.29%   +0.09%     
==========================================
  Files          83       84       +1     
  Lines        6500     6506       +6     
==========================================
+ Hits         6253     6265      +12     
+ Misses        247      241       -6

[micbou]

Reviewed 5 of 7 files at r1, 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 86 at r2 (raw file):

                                         translation_unit_created );

  if ( !unit )

We don't need this check and the others below anymore since the TranslationUnit pointer should always be set in GetOrCreate (an exception is thrown otherwise).

---

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 100 at r2 (raw file):

  }

  return std::vector< Diagnostic >();

Cannot be reached.

---

cpp/ycm/tests/ClangCompleter/ClangCompleter_test.cpp, line 259 at r2 (raw file):

                                                 1,
                                                 unsaved_files,
                                                 flags ) );

These tests are obsolete; they all raise the same std::runtime_error exception.

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

cpp/ycm/exceptions.h, line 0 at r2 (raw file):
What was the reason for replacing the specific exception class ClangParseError with the generic exception type std::runtime_error? Bikeshedding again, but std:runtime_error does not feel like the correct thing to use:

Defines a type of object to be thrown as exception. It reports errors that are due to events beyond the scope of the program and can not be easily predicted.

I think we can easily predict that the errors are due to parsing the code, because that's what the API tells us.

The CPP Core Guidelines say explicitly not to do this, so I think we should derive a new exception type.

---

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 87 at r2 (raw file):

  try {
    return unit->Reparse( unsaved_files );
  } catch ( std::runtime_error & ) {

should probably catch by const reference

---

cpp/ycm/ClangCompleter/ClangUtils.cpp, line 46 at r2 (raw file):
Given that we don't actually need to use this as a string, I think we should return const char * from this method. That way there's no need to perform a heap allocation into the std:string, followed by a copy into the std::runtime_error.

According to the docs:

Because copying std::exception is not permitted to throw exceptions, this message is typically stored internally as a separately-allocated reference-counted string. This is also why there is no constructor taking std::string&&: it would have to copy the content anyway.

This means the line throw std::runtime_error( CXErrorCodeToString( code ) ) requires:

• a ton of compiler-generated stack unwind nonesense
• a heap allocation and memcpy to create the temporary std::string
• a heap allocation and memcpy to create the "internal reference counted string"
• deallocation of the std::string temporary

OTOH a const char * return requires only:

• a ton of compiler-generated stack unwind nonesense
• a heap allocation and memcpy to create the "internal reference counted string"

I realise this is pedantic bikeshedding. So feel free to ignore.

---

cpp/ycm/ClangCompleter/ClangUtils.cpp, line 60 at r2 (raw file):

             "while parsing the translation unit.";
    default:
      return "Unknown error while parsing the translation unit.";

Is this reachable? I tend to prefer to not have a default clause and have the compiler tell us when we haven't covered all of the enum values. Adding a default clause prevents that. For example, if a new code is added in libclang, and we upgrade this default clause would get exercised, but otherwise it's not possible. OTOH if we don't have the default clause, we would get a nice compiler warning that there is a missing case statement.

More bikeshedding.

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

cpp/ycm/ClangCompleter/ClangUtils.cpp, line 46 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Given that we don't actually need to use this as a string, I think we should return const char * from this method. That way there's no need to perform a heap allocation into the std:string, followed by a copy into the std::runtime_error.

According to the docs:

Because copying std::exception is not permitted to throw exceptions, this message is typically stored internally as a separately-allocated reference-counted string. This is also why there is no constructor taking std::string&&: it would have to copy the content anyway.

This means the line throw std::runtime_error( CXErrorCodeToString( code ) ) requires:

• a ton of compiler-generated stack unwind nonesense
• a heap allocation and memcpy to create the temporary std::string
• a heap allocation and memcpy to create the "internal reference counted string"
• deallocation of the std::string temporary

OTOH a const char * return requires only:

• a ton of compiler-generated stack unwind nonesense
• a heap allocation and memcpy to create the "internal reference counted string"

I realise this is pedantic bikeshedding. So feel free to ignore.

FWIW https://godbolt.org/g/vku1G3

---

Comments from Reviewable

[bstaletic]

Reviewed 7 of 7 files at r1, 1 of 3 files at r2, 5 of 8 files at r3.
Review status: 7 of 10 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

cpp/ycm/ClangCompleter/ClangUtils.cpp, line 60 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Is this reachable? I tend to prefer to not have a default clause and have the compiler tell us when we haven't covered all of the enum values. Adding a default clause prevents that. For example, if a new code is added in libclang, and we upgrade this default clause would get exercised, but otherwise it's not possible. OTOH if we don't have the default clause, we would get a nice compiler warning that there is a missing case statement.

More bikeshedding.

Not having a default bit me once, but that's a story for some other place, not a PR review.

---

Comments from Reviewable

[micbou]

Reviewed 7 of 8 files at r3, 2 of 2 files at r4, 4 of 4 files at r5.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

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

Previously, puremourning (Ben Jackson) wrote…

What was the reason for replacing the specific exception class ClangParseError with the generic exception type std::runtime_error? Bikeshedding again, but std:runtime_error does not feel like the correct thing to use:

Defines a type of object to be thrown as exception. It reports errors that are due to events beyond the scope of the program and can not be easily predicted.

I think we can easily predict that the errors are due to parsing the code, because that's what the API tells us.

The CPP Core Guidelines say explicitly not to do this, so I think we should derive a new exception type.

Reasons were:

• Boost automatically translates C++ exceptions to RuntimeError in Python;
• translating a custom C++ exception to its Python counterpart needs extra work;
• std::runtime_error accepts a message.

I updated the PR to use a custom exception. In addition to translate ClangParseError to its Python equivalent, we need to expose the exception to the Python layer (done through the CreatePythonException function). Otherwise, we can't catch the exception in Python.

---

cpp/ycm/ClangCompleter/ClangCompleter.cpp, line 87 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

should probably catch by const reference

Done.

---

cpp/ycm/ClangCompleter/ClangUtils.cpp, line 46 at r2 (raw file):

Previously, puremourning (Ben Jackson) wrote…

FWIW https://godbolt.org/g/vku1G3

That's convincing. Done.

---

cpp/ycm/ClangCompleter/ClangUtils.cpp, line 60 at r2 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Not having a default bit me once, but that's a story for some other place, not a PR review.

The code won't compile in debug mode if there is a missing case and no default (because of -Wall implying -Wswitch, and -Werror) so it's safe to remove the default clause. However, the function still needs to return a string in other cases or GCC will complain with the following error:

ycmd/cpp/ycm/ClangCompleter/ClangUtils.cpp:60:1: error: control reaches end of non-void function [-Werror=return-type]

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

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

PyObject* CreatePythonException( const char* name,

I'm kinda unsure what this is doing and why. I'm not super familiar with the python binding stuff here, but it seems to be injecting an exception object into a scope? Sorry, can't brain today and struggling to get it!

If our exception inherits from std::exception then i would have thought that this would work exactly the same as when it threw std:;exception ?

---

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

  std::string scope_name = extract< std::string >( scope().attr( "__name__" ) );
  std::string qualified_name = scope_name + "." + name;
  char *raw_name = const_cast< char * >( qualified_name.c_str() );

on the surface, this looks pretty evil.

Why the need for the const_cast here? In particular, we're passing this (presumably as char *) into the PyErr_NewException, but we don't own it and we can't modify it.

I'm expecting that PyErr_NewException doesn't really want to modify the string pointed to by raw_name and is just using a c interface, so it is probably safe, but maybe a quick note to that effect?

I

---

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

#ifdef USE_CLANG_COMPLETER
PyObject* PyExc_ClangParseError = NULL;

This global with no storage class is irksome. Do we really need global variable here? Should it at least be in this file's unnamed namespace? That would at least give it TU scope, rather than global.

I must confess, I'm a little perplexed why TranslateClangParseError sets this, and how it is used. Probably at least needs a comment.

---

cpp/ycm/ClangCompleter/ClangUtils.h, line 46 at r5 (raw file):

 * Thrown when libclang fails to parse (or reparse) the translation unit.
 */
struct ClangParseError : virtual std::runtime_error {

woah. virtual inheritance... that's unusual. This is like an interview question in a code review !

Fortunately, unless i'm very much mistaken, this is all single-inheritance, and there is no dreaded diamond, so we don't actually need to virtually inherit here. Pretty sure std::runtime_error just inherits from std::exception so we're all good with:

struct ClangParseError : public std::runtime_error {
...
};

And then none of us has to explain how RTTI works and the stable layout for a CatDog which is a Cat and is a Dog and both Cat and Dog are Animals and we can pretend that we just know, and it will all be fine.

---

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 105 at r5 (raw file):

                          EditingOptions(),
                          &clang_translation_unit_ );
  if ( failure )

I actually prefer to use the explicit check on CXError_Success. It's clearer and how the API is intended (otherwise they'd return int). Implicit narrowing casts (such as this one to bool) are IMO a pretty regrettable language feature.

---

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 372 at r5 (raw file):

  }

  if ( failure ) {

I'd prefer to compare with a value in the enum.

---

Comments from Reviewable

[micbou]

Reviewed 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

cpp/ycm/ycm_core.cpp, line 58 at r5 (raw file):
Moved the code to the new PythonException templated class.

If our exception inherits from std::exception then i would have thought that this would work exactly the same as when it threw std:;exception ?

By default, Boost.Python translates all C++ exceptions to RuntimeError in Python. So, we need to use the register_exception_translator function to translate ClangParseError into its Python equivalent. However, this doesn't allow us to catch the exception in Python. For that, we need to add it to the ycm_core module as a Python object so that we can catch it like this:

try {
  ...
} except ycm_core.ClangParseError {
  ...
}

---

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

Previously, puremourning (Ben Jackson) wrote…

on the surface, this looks pretty evil.

Why the need for the const_cast here? In particular, we're passing this (presumably as char *) into the PyErr_NewException, but we don't own it and we can't modify it.

I'm expecting that PyErr_NewException doesn't really want to modify the string pointed to by raw_name and is just using a c interface, so it is probably safe, but maybe a quick note to that effect?

I

The signature of PyErr_NewException is char *name, PyObject *base, PyObject *dict in Python 2 (but const char *name, PyObject *base, PyObject *dict in Python 3) so we have to cast away the constness (for Python 2). Added a comment in PythonException.

---

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

Previously, puremourning (Ben Jackson) wrote…

This global with no storage class is irksome. Do we really need global variable here? Should it at least be in this file's unnamed namespace? That would at least give it TU scope, rather than global.

I must confess, I'm a little perplexed why TranslateClangParseError sets this, and how it is used. Probably at least needs a comment.

Now a private member of PythonException.

---

cpp/ycm/ClangCompleter/ClangUtils.h, line 46 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

woah. virtual inheritance... that's unusual. This is like an interview question in a code review !

Fortunately, unless i'm very much mistaken, this is all single-inheritance, and there is no dreaded diamond, so we don't actually need to virtually inherit here. Pretty sure std::runtime_error just inherits from std::exception so we're all good with:

struct ClangParseError : public std::runtime_error {
...
};

And then none of us has to explain how RTTI works and the stable layout for a CatDog which is a Cat and is a Dog and both Cat and Dog are Animals and we can pretend that we just know, and it will all be fine.

That's what happens when you copy paste code without understanding it. Thanks for the link. Given that a struct has public inheritance by default, I simply removed the virtual keyword.

---

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 105 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I actually prefer to use the explicit check on CXError_Success. It's clearer and how the API is intended (otherwise they'd return int). Implicit narrowing casts (such as this one to bool) are IMO a pretty regrettable language feature.

At least, I learned it was working this way. Done.

---

cpp/ycm/ClangCompleter/TranslationUnit.cpp, line 372 at r5 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I'd prefer to compare with a value in the enum.

Done.

---

Comments from Reviewable

[bstaletic]

To be frank, I'm not a fan of using python's C API directly. Yet it looks like we don't have too much choice if we want to keep using exceptions like we alwas had.

So, after having explained what @puremourning asked about the C API, this is .

---

Reviewed 1 of 2 files at r4, 3 of 4 files at r5, 4 of 4 files at r6.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

cpp/ycm/PythonSupport.h, line 65 at r6 (raw file):

    // Add the Python exception to the current Boost.Python module.
    boost::python::scope().attr( name ) = boost::python::handle<>(

OK so I read up on the python API and boost python as best I could to understand this.

What I was trying to work out was this: we create this python_exception_ thing, but who frees it, and when?

I came to the conclusion that I was not sure whether or not we need to decrement the reference count for the the exception object in the destructor of this class. It's frustratingly difficult to be sure.

I was leaning towards thinking that perhaps the member object should be a boost::python::handle<PyObject> instance ? Then the assignment into the module would just be just a copy of the handle and the destructor of this class would decrement the reference count, and there's no chance that it gets freed before we call our operator() and try and use it.

I honestly don't know if that's likely/possible, but this line seems odd in that it allocates and immediately deallocates a smart pointer object wrapping python_exception_ ; that seems odd as it will increment and then (almost) immediately decrement the reference count.

Sorry that's waffly, but I'm really not that familiar with this API and the docs are categorically abysmal.

---

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

Previously, micbou wrote…

Moved the code to the new PythonException templated class.

If our exception inherits from std::exception then i would have thought that this would work exactly the same as when it threw std:;exception ?

By default, Boost.Python translates all C++ exceptions to RuntimeError in Python. So, we need to use the register_exception_translator function to translate ClangParseError into its Python equivalent. However, this doesn't allow us to catch the exception in Python. For that, we need to add it to the ycm_core module as a Python object so that we can catch it like this:

try {
  ...
} except ycm_core.ClangParseError {
  ...
}

Got it, thanks!

P.S. Tee hee. The example syntax in your comment is like the scary lovechild of c++ and python!

---

cpp/ycm/ycm_core.cpp, line 55 at r6 (raw file):

The signature of PyErr_NewException is char *name, PyObject *base, PyObject *dict in Python 2 (but const char

---

cpp/ycm/ClangCompleter/ClangUtils.cpp, line 55 at r6 (raw file):

      return "Libclang crashed while parsing the translation unit.";
    case CXError_InvalidArguments:
      return "Parsing the translation unit with invalid arguments.";

This message doesn't seem to make sense in isolation. Perhaps it should say "Invalid compiler flags supplied for the translation unit." ?

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

cpp/ycm/PythonSupport.h, line 76 at r6 (raw file):

private:
  PyObject* python_exception_;

initialise to nullptr?

PyObject* python_execption_{ nullptr };

Shrug.

---

Comments from Reviewable

[puremourning]

Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

---

cpp/ycm/PythonSupport.h, line 65 at r6 (raw file):

Previously, puremourning (Ben Jackson) wrote…

OK so I read up on the python API and boost python as best I could to understand this.

What I was trying to work out was this: we create this python_exception_ thing, but who frees it, and when?

I came to the conclusion that I was not sure whether or not we need to decrement the reference count for the the exception object in the destructor of this class. It's frustratingly difficult to be sure.

I was leaning towards thinking that perhaps the member object should be a boost::python::handle<PyObject> instance ? Then the assignment into the module would just be just a copy of the handle and the destructor of this class would decrement the reference count, and there's no chance that it gets freed before we call our operator() and try and use it.

I honestly don't know if that's likely/possible, but this line seems odd in that it allocates and immediately deallocates a smart pointer object wrapping python_exception_ ; that seems odd as it will increment and then (almost) immediately decrement the reference count.

Sorry that's waffly, but I'm really not that familiar with this API and the docs are categorically abysmal.

No I think i'm talking rubbish.

---

cpp/ycm/ycm_core.cpp, line 98 at r6 (raw file):

#ifdef USE_CLANG_COMPLETER
  PythonException< ClangParseError >( "ClangParseError" );

This call instantiates the class PythonException but that instance goes out of scope at the end of this function.
Does that mean that the callback ends up pointing at garbage after we leave this function ?

---

Comments from Reviewable

[zzbot]

☔ The latest upstream changes (presumably #900) made this pull request unmergeable. Please resolve the merge conflicts.

[zzbot]

☔ The latest upstream changes (presumably #909) made this pull request unmergeable. Please resolve the merge conflicts.

[micbou]

Reviewed 2 of 2 files at r10, 1 of 1 files at r11, 3 of 3 files at r12.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

cpp/ycm/PythonSupport.h, line 60 at r9 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

To be honest, @puremourning made me unsure how okay this is.

As far as I know, when using python's C API, the programmer himself is responsible for reference counting.
That's why there are Py_INCREF and Py_DECREF (and others).
Here's a link: https://docs.python.org/2/c-api/refcounting.html

I removed the call to boost::python::borrowed when adding the exception to the module. This was probably resulting in a memory leak as that call was incrementing the reference count which in turn was never decremented. See https://wiki.python.org/moin/boost.python/handle. I am still not sure there is no memory leak but at least I know that if there is one, it's not a leak that leads to a continuous increase in memory usage when repeatedly raising and catching the exception. Therefore, I am marking the PR as ready.

---

Comments from Reviewable

[bstaletic]

Reviewed 1 of 2 files at r9, 2 of 2 files at r10, 1 of 1 files at r11, 3 of 3 files at r12.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

cpp/ycm/PythonSupport.h, line 60 at r9 (raw file):

Previously, micbou wrote…

I removed the call to boost::python::borrowed when adding the exception to the module. This was probably resulting in a memory leak as that call was incrementing the reference count which in turn was never decremented. See https://wiki.python.org/moin/boost.python/handle. I am still not sure there is no memory leak but at least I know that if there is one, it's not a leak that leads to a continuous increase in memory usage when repeatedly raising and catching the exception. Therefore, I am marking the PR as ready.

Since @puremourning recently confirmed this is working, this does get a green light from me.

---

Comments from Reviewable

[micbou]

Improved coverage and added comments in the tests explaining why libclang fails to parse (or reparse) the translation unit.

---

Reviewed 6 of 6 files at r13, 3 of 3 files at r14.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bstaletic]

I believe we can merge this now.

---

Reviewed 4 of 6 files at r13, 3 of 3 files at r14.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[puremourning]

Thanks. I did confirm this works well

@zzbot r+

---

Reviewed 1 of 7 files at r1, 1 of 8 files at r3, 1 of 4 files at r6, 1 of 2 files at r9, 1 of 1 files at r11, 3 of 3 files at r12, 3 of 6 files at r13, 3 of 3 files at r14.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

cpp/ycm/ycm_core.cpp, line 98 at r6 (raw file):

Previously, micbou wrote…

I find it really convenient that we only have to instantiate an object to translate a C++ exception into Python. It won't be as clean if we add a method for that.

ok

---

Comments from Reviewable

[zzbot]

📌 Commit 7033503 has been approved by puremourning

[zzbot]

⌛ Testing commit 7033503 with merge 165e00f...

[zzbot]

💔 Test failed - status-appveyor

[zzbot]

☀️ Test successful - status-appveyor, status-travis
Approved by: puremourning
Pushing 165e00f to master...