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

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

Closed
wants to merge 1 commit into from

Conversation

Sarcasm
Copy link

@Sarcasm Sarcasm commented Dec 2, 2016

This change adds an exception to the using namespace directives check,
to allow user-defined literals namespaces for source files (not headers).

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@Sarcasm
Copy link
Author

Sarcasm commented Dec 2, 2016

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@tituswinters
Copy link
Contributor

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

@Sarcasm
Copy link
Author

Sarcasm commented Dec 2, 2016

Understood, thanks for the feedback.

@wjwwood
Copy link

wjwwood commented Dec 8, 2016

@tituswinters does that apply to all using directives or just using namespace directives?


I can understand the guidance on never using the using namespace directive, even for user-defined literals.

However, for the very narrow and specific case of std library defined literals, I think it should be acceptable to do something like using namespace std::chrono_literals, simple because these literals cannot be intentionally or unintentionally ambiguous. See:

All ud-suffixes introduced by a program must begin with the underscore character _. The standard library ud-suffixes do not begin with underscores.

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

So only the std library can define literals like operator ""s.

It is also a convenience feature that cannot be used conveniently without the using namespace directive, which is the recommendation for std::literals::chrono_literals::operator""s (as an example):

Access to these operators can be gained with using namespace std::literals, using namespace std::chrono_literals, and using namespace std::literals::chrono_literals.

-- http://en.cppreference.com/w/cpp/chrono/operator%22%22s#Notes

So, the std library wisely decided to not make these literals accessible by default, to avoid surprise I would guess, but they intend to expose the feature through the use of using namespace. If you still don't see the need for the exemption, then perhaps the stance of the linter should be to not allow user defined literals at all.

@tituswinters
Copy link
Contributor

@wjwwood - Using directives are of the form "using namespace blah;"
Using declarations are of the form "using myproject::some_symbol;"

I'm only talking about directives.

For the specific case of the standard-defined UDLs which can (allegedly) never collide with anything else, I agree that this would be fine. However, in our experience anything that weakens the limitation on using directives leads pretty directly to abuse, and the cost to a large repo for cleaning up that abuse is substantial. We distrust using directives at scale sufficiently that we're OK throwing out ease-of-use for UDLs at the same time. (The "allegedly" in there is really because non-standard UDLs are not allowed to not be prefixed with a _, but that doesn't mean it won't happen.)

Part of that is that UDLs are troubling in their own right - any signfiicant use of UDLs requires using directives or using declarations to the point that every UDL in a codebase is effectively sharing the same UDL logical namespace. For small and medium projects that might be fine - for large shared codebases it's a recipe for collision, confusion, and subtlely hard to maintain code. The benefits aren't there.

@wjwwood
Copy link

wjwwood commented Dec 8, 2016

I'm only talking about directives.

Got it, thanks for the clarification.

The "allegedly" in there is really because non-standard UDLs are not allowed to not be prefixed with a _, but that doesn't mean it won't happen.

At least in GCC, Clang, and VS 2015 Update 3, a UDL without a leading _ results in a warning. I guess an exotic compiler may not provide a warning for this. Also, the std library could define different versions of the same UDL's in different namespaces. So I think that it's possible, as you said, that some conflict could arise. However, in order for this to happen another project would need to ignore a serious warning or the std library would have to conflict with itself, and so I think it's unlikely to happen, though that's only my opinion.

The benefits aren't there.

I understand your reasoning, thanks for spending the time to express it.

One last question, I promise to stop wasting your time after that 😄, is there a standing policy for this repository on pull requests that add options to diverge from the accepted Google style? For example, if I were to create a pull request which added an option to classify using directives that match std::*literals as a new rule (say [build/std_literals_namespace]), would that be considered for merge?

That way the default behavior would be unchanged, but we could enable that option and ignore just the new rule, rather than ignoring all [build/namespaces] with the filter option.

@tituswinters
Copy link
Contributor

I don't think there's policy on it, but in general we avoid such things because the maintenance cost is a hassle, and we aren't big on supporting configurations that we don't have good ways to test. That could conceivably change, but we're in the middle of shuffling a lot of this stuff - so I'd be a bit surprised if we changed that policy right now.

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

Successfully merging this pull request may close these issues.

4 participants