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

Monkey patch in support for C++11 headers #56

Open
abencz opened this issue Dec 9, 2016 · 3 comments
Open

Monkey patch in support for C++11 headers #56

abencz opened this issue Dec 9, 2016 · 3 comments

Comments

@abencz
Copy link

abencz commented Dec 9, 2016

C++11 headers such as <future> are "unapproved" in upstream cpplint.py. Support for these should be monkey-patched in.

@mikepurvis
Copy link
Member

Problem line: https://github.com/google/styleguide/blob/db0a26320f3e930c6ea7225ed53539b4fb31310c/cpplint/cpplint.py#L5770

Maybe we just disable FlagCxx11Features and FlagCxx14Features altogether? @alexhenning Thoughts?

For now you can // NOLINT, as discussed here: http://wiki.ros.org/roslint#Tips

@alexhenning
Copy link
Contributor

The google style guide seems to mostly allow C++11, I'm not sure how much this has changed since we last synced in cpplint a year and a half ago. Updating to the latest cpplint may reduce these warnings. One of the listed concerns, compiler support for C++11, isn't a problem with commonly used ROS configuration AFAIK. Additionally, It seems that with respect to ROS, ROS2 is using C++11 and it would be nice to share code without unnecessary warings. So, it seems like either updating cpplint (if c++11 warnings have been reduced) or disabling C++11 lints would be a good idea.

In the meantime, you should be able to pass in filter arguments to ignore entire classes of errors if you don't want to litter your code with a lot of // NO LINT statements. The following should work, but I haven't tested it.

set(ROSLINT_CPP_OPTS "--filter=+,-build/c++11")

Relevant excerpt on C++11 from https://google.github.io/styleguide/cppguide.html#C++11

Use libraries and language extensions from C++11 when appropriate. Consider portability to other environments before using C++11 features in your project.

Definition
C++11 contains significant changes both to the language and libraries.

Pros
C++11 was the official standard until august 2014, and is supported by most C++ compilers. It standardizes some common C++ extensions that we use already, allows shorthands for some operations, and has some performance and safety improvements.

Cons
The C++11 standard is substantially more complex than its predecessor (1,300 pages versus 800 pages), and is unfamiliar to many developers. The long-term effects of some features on code readability and maintenance are unknown. We cannot predict when its various features will be implemented uniformly by tools that may be of interest, particularly in the case of projects that are forced to use older versions of tools.

As with Boost, some C++11 extensions encourage coding practices that hamper readability—for example by removing checked redundancy (such as type names) that may be helpful to readers, or by encouraging template metaprogramming. Other extensions duplicate functionality available through existing mechanisms, which may lead to confusion and conversion costs.

Decisions
C++11 features may be used unless specified otherwise. In addition to what's described in the rest of the style guide, the following C++11 features may not be used:

  • Compile-time rational numbers (), because of concerns that it's tied to a more template-heavy interface style.
  • The and <fenv.h> headers, because many compilers do not support those features reliably.
  • Ref-qualifiers on member functions, such as void X::Foo() & or void X::Foo() &&, because of concerns that they're an overly obscure feature.

@mikepurvis
Copy link
Member

mikepurvis commented Dec 9, 2016

My link was to the current upstream of cpplint.py— it definitely still warns about it. So if we want the warning gone, we have to either drop that entire check or rewrite it (or petition upstream to move the list of banned headers to a global that can be more easily patched).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants