move user-defined literals in their own namespace

[Sarcasm]

Following the discussion from #282 (comment)

There are 3 commits:

1. The first one is not supposed to break the examples and tests.
2. The second one "depollutes" the global namespace, this one require the example and tests to be updated.
   The tests and examples will come in separate pull requests.
3. The third one, uses a constexpr implementation as seen on cppreference.com

Connects to #282

[wjwwood]

Cool, thanks!

I added the Connects to ros2/rclcpp#282 to the end of each of your pr descriptions to "link" them to the original issue in our issue tracker: https://waffle.io/ros2/ros2

I'll start some CI for you shortly.

[wjwwood]

CI:

• Linux:
  • 
• OS X:
  • 
• Windows:
  • 

[wjwwood]

Apparently one of our linters (cpplint.py) does not like the using namespace directive. I can somewhat understand that, but honestly I think this case should be fine. I guess we can either:

• disable the warning globally
• disable the warning at each of the lines
• update to use the more specific using rclcpp::literals::operator ""_ms or whatever in each case

I'd vote for the last, since it's unlikely to result in needing multiple lines in the examples and avoids doing nasty things like disabling linter warnings.

I would prefer to merge this contribution in some form, even if we're thinking about converting to the chrono literals with C++14.

[wjwwood]

@Sarcasm note that if you're interested in reproducing these linter errors, you can just run ament_cpplint path/to/directory/to/lint.

[Sarcasm]

Arf, I passed the linters on rclcpp itself but not on the other changes.

I agree it's sad to disable the warning globally.
But I think I prefer a nolint comment than using the full using declaration because IMHO that's how the user-defined literals namespace are supposed to be used.
The using-declaration is difficult to come up from the top of the head IMHO. Making the examples actually more difficult to grasp.
Because it looks too complex, I no longer see the benefits of the literal for some examples, that uses the literal only once.

using rclcpp::literals::operator ""_ms;

void f() {
   spin_some(30_ms);
}

VS

void f() {
   spin_some(std::chrono::milliseconds(30));
}

What do you think of whitelisting literals and *_literals namespace in cpplint?
I can try that, it would be reasonable IMHO.

[wjwwood]

Seeing as we'll need a similar using namespace std::chrono::literals, it makes sense to me to try and make an exception in the cpplint.py checking for this kind of statement. What do others think?

[wjwwood]

@Sarcasm a no lint comment would be ok for me, since I think it will be temporary because we're going to probably do the switch to C++14 soon.

We may or may not have need of the "using namespace .*literals;" exclusion in cpplint.py once we start using std::chrono's literals.

[Sarcasm]

Ok, I can do that.
I already implemented the exclusion in cpplint.py, should I propose a PR or is it better to leave this alone?

[wjwwood]

Up to you, worst case we decline the cpplint pr.

[Sarcasm]

I updated all the PRs with no lint comments.

And the cpplint proposal is here:

• cpplint: allow using-directive for user-defined literals namespaces ament/ament_lint#67

[dirk-thomas]

I would still avoid using namespace directives in headers since they are a side effect which should be avoided. It comes to a surprise to the user and that is why the general recommendation is not to do it.

Imo the using declaration is preferred. If a file uses the literal only once then it is up to the developer to choose if the using declaration is worth it or to use the full symbol instead.

I also think that we should avoid adding further patches into our cpplint fork. I would rather aim to use upstream at some point in the future and this detaches us even further.

[wjwwood]

I would still avoid using namespace directives in headers since they are a side effect which should be avoided. It comes to a surprise to the user and that is why the general recommendation is not to do it.

I do not believe that was suggested here. Except in the intermediate commit which restored existing behavior. That was removed in the last commit.

Imo the using declaration is preferred. If a file uses the literal only once then it is up to the developer to choose if the using declaration is worth it or to use the full symbol instead.

I agree that the using statement is preferable in pretty much all cases, i.e. using std::string; using std::vector; over using namespace std;, but in the case of the using namespace std::chrono::literals I think it's ok because the operators are inconvenience to specify using the stricter using ... statement and because the literal suffixes defined therein cannot collide with user code by design. See:

http://en.cppreference.com/w/cpp/language/user_literal

ud-suffix - an identifier, introduced by a literal operator or a literal operator template declaration (see below). All ud-suffixes introduced by a program must begin with the underscore character _. The standard library ud-suffixes do not begin with underscores.

So in this very narrow case, I think it's ok to use the using namespace directive.

I also think that we should avoid adding further patches into our cpplint fork. I would rather aim to use upstream at some point in the future and this detaches us even further.

I agree with that. The correct thing to do would be to propose an upstream patch which gives us the option to ignore this linter error if we don't agree with it in every case, or a way to configure cpplint.py to allow the exact line using namespace std::chrono::literals; despite it being a using namespace statement.

[wjwwood]

Even the cppreference website's documentation suggests that you use the using namespace directive (in one of a few ways) to get access to the literal suffixes:

http://en.cppreference.com/w/cpp/chrono/operator%22%22h

These operators are declared in the namespace std::literals::chrono_literals, where both literals and chrono_literals are inline namespaces. Access to these operators can be gained with using namespace std::literals, using namespace std::chrono_literals, and using namespace std::literals::chrono_literals.
In addition, within the namespace std::chrono, the directive using namespace literals::chrono_literals; is provided by the standard library, so that if a programmer uses using namespace std::chrono; to gain access to the duration classes, the duration literal operators become visible as well.

Though they do limit the scope of the using namespace statement to the main function in their examples, which is something we can do. It would also be another way that we could propose cpplint be more flexible: allow these statements in confined scopes. In fact it might already do that, but I'm not sure.

[Sarcasm]

I would still avoid using namespace directives in headers since they are a side effect which should be avoided. It comes to a surprise to the user and that is why the general recommendation is not to do it.

You are totally right I can fix my cpplint.py PR to disable the additional checks in headers but if your goal is to use upstream at one point, I think it's best to close the PR. Maybe I can propose upstream.

I checked the code a bit more.
There are 2 situations right now:

1. examples and demos: have usually 1 use of a literal per file
2. system_tests: can have a bunch bunch

For the examples and demos, I think the best is to keep them as simple as possible meaning the NOLINT comment would be better avoided and (personal opinion here) the using directive should also be avoided.
If cpplint.py is not a good option then I think it's reasonable to just use things like std::chrono::milliseconds(...).

For system_tests, the NOLINT comment seems reasonable.

Regarding the scope of the using namespace, I can refine the scope but I think in system_tests it will look better for the whole file.

[Sarcasm]

For the record, Google would not accept the upstream change:

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]

I've updated all of these pr's to use C++14 and remove references to the rclcpp literals we had before. Looking for reviews and here's CI:

• Linux:
  • 
• OS X:
  • 
• Windows:
  • 

[wjwwood]

Doh, I thought I could get away with the old gist I used before. Here's new CI with an updated repos file:

• Linux:
  • 
• OS X:
  • 
• Windows:
  • 

[wjwwood]

I had to rebase this one to fix the CI.

[wjwwood]

Ok, finally working. The only remaining warning is in the tutorial package, the allocator example, which we're excluding also, I don't know how to fix it right off but it's related to allocators and c++14 and the new "sized delete". CI:

• Linux:
  • 
• OS X:
  • 
• Windows:
  • 

[wjwwood]

I'm going to merge these because they are blocking the other prs and the changes are relatively straightforward. If anyone has some issues or comments I can address them in a follow up pr.

[dirk-thomas]

@wjwwood This broke the packaging job / ros1_bridge.

[wjwwood]

@dirk-thomas sorry about that, fix is in: ros2/ros1_bridge#52

[dirk-thomas]

👍