cpplint: allow using-directive for user-defined literals namespaces

[Sarcasm]

As proposed here:

• move user-defined literals in their own namespace ros2/rclcpp#284 (comment)

Related to ros2/rclcpp#284.

[wjwwood]

Thanks, we'll see if we end up needing this or not.

I removed the "Connects to" because this will not be needed for the other pr's since you added the nolint lines.

[codebot]

Greetings! Thanks for this PR. Due to this gcc warning bug, we don't see any other way to allow use of the C++14 literals while also using cpplint. So we made this check somewhat more specific by ensuring the *literals namespace is coming from the std:: prefix.

[dirk-thomas]

The patch seems to change the access permissions of the file which is probably undesired.

[dirk-thomas]

For completeness I will repeat the comment we received from upstream about considering to propose this change to them:

We (internally) had a long discussion recently and came to the conclusion that using directives are never ever ever ever safe for use in a codebase of any significant size - we cannot endorse this, even just once. Sorry.

(I'm working on finding a proper forum to share some of the Google-internal guidance, hopefully early next year.)

-- google/styleguide#204 (comment)

[wjwwood]

Anyone following this should also read the follow up comment specifically about using namespace std::*literals:

google/styleguide#204 (comment)

[codebot]

OK. Well, this leaves our transition to standard C++14 literals then kind of stuck at the moment, due to conflicting ideals:

1. never allow a using-declaration
2. never allow a warning on CI that we merge

item 1 happens because we're trying not to diverge (too far) from the Google style guide

item 2 happens because, at least to my knowledge, there is no way to use the C++14 std::chrono literals that does not result in a gcc warning other than using namespace std::chrono_literals; because of the gcc warning bug linked above.

[codebot]

@dirk-thomas thanks for pointing that out. The file permission change is now reverted.

[Sarcasm]

If restricting to the STL is decided, then the comments should also be updated.

[codebot]

Thanks @Sarcasm for pointing out the inconsistency in the code and comment. The comment is updated now.

[Sarcasm]

Hello @codebot,

TBH, I'm glad GCC has this warning bug because I don't consider the use of the using declaration elegant (personal taste).

Regarding restricting the match to std:: members, I'm not a big fan either but you are the judges.
In the following PR ros2/rclcpp#284,
I'm adding a user defined literal namespace to rclcpp.
I'm trying to follow the STL convention of a namespace named with literals, which I think is a convention quite a few people would follow naturally. I estimate the number of literals namespaces that will be abused by people in the ros2 codebase, without people noticing in the review, to be very close to 0 (risky estimate). Personally I think UDL are nice, for test code, examples, a bit less on real-world code, and I don't think the use should be limited to the STL, if Boost, or any other library defines some lietrals in a namespace, it makes sense to allow them when then follow the STL pattern of naming the namespace *literals.

I haven't given extensive thoughts on the subject but I'm wondering if the std::placeholders namespace could not be allowed as well.

Regarding the flexibility of the linting tool.
I'm wondering if clang-tidy (http://clang.llvm.org/extra/clang-tidy/checks/google-build-using-namespace.html) would not be more welcoming to configurable rules that diverges from the Google Style.
But I digress, clang-tidy integration would be significantly more difficult to integrate than cpplint.

[codebot]

Thanks @Sarcasm for the feedback. We just altered this so that it uses a whitelist that will be easier to change in the future if/when new STL or Boost other "standard-ish" libraries emerge that are well-behaved and convenient to use in this fashion. We also added std::placeholders as per your suggestion.

[Sarcasm]

👍

[dirk-thomas]

Maybe order the list?

[wjwwood]

@codebot I added a few literals in ea5846f and fixed the alphabetical ordering in 787ae55 (doh)

[wjwwood]

As much as I hate to have patches to cpplint in our project, I think this pr represents the most pragmatic compromise between using new features in the standard as they were intended versus avoiding the abuse of the using directive. So, +1 from me.

@dirk-thomas didn't we have a place to keep track of the changes we have on top of cpplint? I saw the url at the top of the file to the originating commit on the cpplint repository, but not an abbreviated list of changes or anything.

[dirk-thomas]

I don't think such a list exists anywhere beside the commit log (https://github.com/ament/ament_lint/commits/master/ament_cpplint/ament_cpplint/cpplint.py). All customizations are can be found in the commit after the last import from upstream (2324287#diff-b5c57d8cc3ce83e990089f94f5ab6e15).