Resolving C++11 TBD features

[Peter Kasting]

Per https://groups.google.com/a/chromium.org/d/topic/cxx/gv7E_n4AnP4/discussion , we're moving C++11 features to default-allowed, and we need a go/no-go decision on the remaining TBD features.  There is a list on http://chromium-cpp.appspot.com/#core-review.

Features I could imagine concerns over (but please check the full list and yell if I omitted your favorite):

• UTF-x string literals/charXX_t support - we're in the process of trying to start using these, so it seems like churn to disallow them only to allow them very soon
• Aligned storage features - toolchain/compat concerns, but maybe we don't care about MSVC2017 or google3 cross-compiling toolchains
• std::stod() - not clear if we want to allow this while dmg_fp is still in use
• std::stoi() etc./std::to_string() - presumably replaces base/strings/string_number_conversions.h , not clear if we want to allow without a plan to deprecate that
• std::weak_ptr - kinda useless when we ban std::shared_ptr (though I wonder if we should revisit that decision), though I guess maybe unnecessary to explicitly ban

I think the UTF string literals and charXX_t types are fine, and failing to ban std::weak_ptr is harmless.  I'm weakly inclined to say we don't care about MSVC 2017 and move std::aligned_storage to the allowed list, and let the other aligned storage stuff in.  I think we should allow stoi()/to_string() with a separate bug on replacing our existing string/number conversions functions with these.  stod() seems like the scariest.  I would probably explicitly ban with a note saying we'd like to take it as long as someone can do the work to ensure it's safe ( https://bugs.chromium.org/p/chromium/issues/detail?id=593512 ).

[Jeremy Roman]

Maybe base/ owners can weigh in on the relative utility of string_number_conversions.h etc? Blink has other its own, but it also has its own string types, so I'm not very familiar with the base ones.

On Fri, Mar 8, 2019 at 5:28 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Per https://groups.google.com/a/chromium.org/d/topic/cxx/gv7E_n4AnP4/discussion , we're moving C++11 features to default-allowed, and we need a go/no-go decision on the remaining TBD features.  There is a list on http://chromium-cpp.appspot.com/#core-review.

Features I could imagine concerns over (but please check the full list and yell if I omitted your favorite):

• UTF-x string literals/charXX_t support - we're in the process of trying to start using these, so it seems like churn to disallow them only to allow them very soon
• Aligned storage features - toolchain/compat concerns, but maybe we don't care about MSVC2017 or google3 cross-compiling toolchains
• std::stod() - not clear if we want to allow this while dmg_fp is still in use
• std::stoi() etc./std::to_string() - presumably replaces base/strings/string_number_conversions.h , not clear if we want to allow without a plan to deprecate that
• std::weak_ptr - kinda useless when we ban std::shared_ptr (though I wonder if we should revisit that decision), though I guess maybe unnecessary to explicitly ban

Probably clearest to ban it explicitly and point to base::WeakPtr, so people familiar with std::weak_ptr know where to look for an alternative.

 

I think the UTF string literals and charXX_t types are fine, and failing to ban std::weak_ptr is harmless.  I'm weakly inclined to say we don't care about MSVC 2017 and move std::aligned_storage to the allowed list, and let the other aligned storage stuff in.  I think we should allow stoi()/to_string() with a separate bug on replacing our existing string/number conversions functions with these.  stod() seems like the scariest.  I would probably explicitly ban with a note saying we'd like to take it as long as someone can do the work to ensure it's safe ( https://bugs.chromium.org/p/chromium/issues/detail?id=593512 ).

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBoihdS0utqSp3HaRUMYJRctkBe_S_a4Gr7RXvEaGPxsw%40mail.gmail.com.

[Kyle Charbonneau]

Maybe base/ owners can weigh in on the relative utility of string_number_conversions.h etc?

In terms of replacing string_number_conversions.h I see a few issues:

1. The STL string-to-number functions rely on exceptions to communicate failure to convert the string to a number (eg. overflow, not an integer value, etc). Chrome has exceptions disabled and crashes for failures. The base::StringToInt() functions handle failure to convert the string gracefully with a return value.
2. The STL number-to-string functions use the current std::locale which is apparently slower. It also changes the output string based on what locale is set.
3. STL string-number conversion functions don't support the base::string16 type (unless WCHAR_T_IS_UTF16). We'd still need the base versions for string16 as a result.

I'd vote towards banning std::stoi() / std::stod() / std::to_string() rather than allowing them since I think those issues would prevent switching all the existing usage to use the STL functions. We could allow both base and STL functions, but there are subtle gotchas for the STL functions, which makes me think banning them entirely would be simpler.

Probably clearest to ban it explicitly and point to base::WeakPtr, so people familiar with std::weak_ptr know where to look for an alternative.

 

 +1

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13eD9OoBpF4NQ2MdPZAz%3DkiZnRqCesa3nU5%2Bw1ZDV%2BHujg%40mail.gmail.com.

[Peter Kasting]

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.
• No discussion of UTF-xx types or aligned storage, so presumably we allow these

Any other comments?  I would be particularly interested in "we still shouldn't allow aligned storage" as I feel like I don't know if we have toolchain pitfalls.  Maybe thakis@ could speak to this?

I'll wait another week before making the above changes.

PK

[Daniel Cheng]

I think MSVC might have fix the std::aligned_storage issue: https://developercommunity.visualstudio.com/content/problem/9629/stdaligned-storage-is-non-compliant-and-nonsensica.html

But I don't have an easy way to test this.

Daniel

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFDHbjTfGH0wktnDryONBEEyG2O9o-CCMVWuTYFokjrSUw%40mail.gmail.com.

[Gabriel Charette]

On Fri, Mar 15, 2019 at 1:05 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.

I saw std::to_string for the first in a review yesterday. Looked up our guidelines and didn't object since we don't ban it and it looks like there are 100s of existing callers in our codebase. The only exception it can throw is std::bad_alloc from the std::string constructor.

 

• No discussion of UTF-xx types or aligned storage, so presumably we allow these

Any other comments?  I would be particularly interested in "we still shouldn't allow aligned storage" as I feel like I don't know if we have toolchain pitfalls.  Maybe thakis@ could speak to this?

I'll wait another week before making the above changes.

PK

--

[Peter Kasting]

On Wed, Mar 20, 2019 at 11:44 AM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Mar 15, 2019 at 1:05 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.

I saw std::to_string for the first in a review yesterday. Looked up our guidelines and didn't object since we don't ban it

We do ban it; it's in "C++11 Standard Library Features To Be Discussed", which says "The following C++ library features are currently disallowed."  All TBD features are banned until approved.  It would be good if you could let your reviewee know that they need to use base::NumberToString() instead.

 

and it looks like there are 100s of existing callers in our codebase.

In fairness, more than half of those are third_party and v8 code.  That leaves about 200 cases, which is higher than I'd hope for a banned feature but not very high in the grand scheme of things for a feature which has no presubmit check against it.  Heck, std::shared_ptr appears 63 times in those same directories and that's much more obviously banned.

The only exception it can throw is std::bad_alloc from the std::string constructor.

Yes, on the exception front to_string() is more OK than e.g. stoi(), but I'm not convinced it makes sense to accept one direction and not the other, and there are still the (more minor IMO) locale and perf issues around it, as well as the lack of string16 support.  If we have to have something like base::to_string16(), we might as well keep what we've got, it seems like.

PK

[Daniel Bratell]

On Wed, 20 Mar 2019 21:07:41 +0100, 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Wed, Mar 20, 2019 at 11:44 AM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Mar 15, 2019 at 1:05 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.

I saw std::to_string for the first in a review yesterday. Looked up our guidelines and didn't object since we don't ban it

We do ban it; it's in "C++11 Standard Library Features To Be Discussed", which says "The following C++ library features are currently disallowed."  All TBD features are banned until approved.  It would be good if you could let your reviewee know that they need to use base::NumberToString() instead.

 

and it looks like there are 100s of existing callers in our codebase.

In fairness, more than half of those are third_party and v8 code.  That leaves about 200 cases, which is higher than I'd hope for a banned feature but not very high in the grand scheme of things for a feature which has no presubmit check against it.  Heck, std::shared_ptr appears 63 times in those same directories and that's much more obviously banned.

Should probably accompany bans with presubmit warnings/errors just to avoid accidents, and to trigger official discussions rather than people disregarding the rules. Any downside to expanding the exiting ban lists in PRESUBMIT.py with std:c functions?

/Daniel

--

/* Opera Software, Linköping, Sweden: CET (UTC+1) */

[Gabriel Charette]

Strong +1; if our reasons to ban are justified, we should aim for 0 instances. It's hard to tell a reviewee not to do something which 200 other sites are already doing (it feels a bit like the style discussions before clang-format).

[Peter Kasting]

My only reasons would be:

• PRESUBMIT is already very slow and it would be nice not to make it slower.  Can we make this a Tricium check or something?
• As someone who's spent a lot of time in the past trying repeatedly to get a presubmit check not to have false positives, it'd be nice not to obligate people who propose style changes to do this work unless it's both trivial and robust.

In principle, I'd always be glad for more automated enforcement of any team rules.

PK

[dan...@chromium.org]

On Thu, Mar 21, 2019 at 11:38 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Thu, Mar 21, 2019 at 3:30 AM Daniel Bratell <bra...@opera.com> wrote:

On Wed, 20 Mar 2019 21:07:41 +0100, 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Wed, Mar 20, 2019 at 11:44 AM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Mar 15, 2019 at 1:05 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.

I saw std::to_string for the first in a review yesterday. Looked up our guidelines and didn't object since we don't ban it

We do ban it; it's in "C++11 Standard Library Features To Be Discussed", which says "The following C++ library features are currently disallowed."  All TBD features are banned until approved.  It would be good if you could let your reviewee know that they need to use base::NumberToString() instead.

 

and it looks like there are 100s of existing callers in our codebase.

In fairness, more than half of those are third_party and v8 code.  That leaves about 200 cases, which is higher than I'd hope for a banned feature but not very high in the grand scheme of things for a feature which has no presubmit check against it.  Heck, std::shared_ptr appears 63 times in those same directories and that's much more obviously banned.

Should probably accompany bans with presubmit warnings/errors just to avoid accidents, and to trigger official discussions rather than people disregarding the rules. Any downside to expanding the exiting ban lists in PRESUBMIT.py with std:c functions?

My only reasons would be:

• PRESUBMIT is already very slow and it would be nice not to make it slower.  Can we make this a Tricium check or something?

It is mostly slow because of the git cl format check, that is IIRC more than 70% of the time. I doubt adding these sorts of things would be noticable. 

• As someone who's spent a lot of time in the past trying repeatedly to get a presubmit check not to have false positives, it'd be nice not to obligate people who propose style changes to do this work unless it's both trivial and robust.

A well written presubmit test could have a list of banned symbols and then we need only add to the list in the future.

 

In principle, I'd always be glad for more automated enforcement of any team rules.

PK

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFB4OrBB5po-Thf_aP%3DoD5BLRsdboNwjJE-g5C7JH19YbQ%40mail.gmail.com.

[Daniel Bratell]

On Thu, 21 Mar 2019 16:40:51 +0100, <dan...@chromium.org> wrote:

On Thu, Mar 21, 2019 at 11:38 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Thu, Mar 21, 2019 at 3:30 AM Daniel Bratell <bra...@opera.com> wrote:

On Wed, 20 Mar 2019 21:07:41 +0100, 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Wed, Mar 20, 2019 at 11:44 AM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Mar 15, 2019 at 1:05 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.

I saw std::to_string for the first in a review yesterday. Looked up our guidelines and didn't object since we don't ban it

We do ban it; it's in "C++11 Standard Library Features To Be Discussed", which says "The following C++ library features are currently disallowed."  All TBD features are banned until approved.  It would be good if you could let your reviewee know that they need to use base::NumberToString() instead.

 

and it looks like there are 100s of existing callers in our codebase.

In fairness, more than half of those are third_party and v8 code.  That leaves about 200 cases, which is higher than I'd hope for a banned feature but not very high in the grand scheme of things for a feature which has no presubmit check against it.  Heck, std::shared_ptr appears 63 times in those same directories and that's much more obviously banned.

Should probably accompany bans with presubmit warnings/errors just to avoid accidents, and to trigger official discussions rather than people disregarding the rules. Any downside to expanding the exiting ban lists in PRESUBMIT.py with std:c functions?

My only reasons would be:

• PRESUBMIT is already very slow and it would be nice not to make it slower.  Can we make this a Tricium check or something?

It is mostly slow because of the git cl format check, that is IIRC more than 70% of the time. I doubt adding these sorts of things would be noticable. 

• As someone who's spent a lot of time in the past trying repeatedly to get a presubmit check not to have false positives, it'd be nice not to obligate people who propose style changes to do this work unless it's both trivial and robust.

A well written presubmit test could have a list of banned symbols and then we need only add to the list in the future.

There is already such a list (actually several lists, one for java, one for mm, one for cpp): https://cs.chromium.org/chromium/src/PRESUBMIT.py?q=PRESUBMIT.p&sq=package:chromium&g=0&l=210

It bans things like NULL, #pragma comment, FRIEND_TEST, std::regex, 

They can be either warnings or errors. I think it's important that errors have no false positives because using try=nope in cq is awful.

It won't be much of an extra burden to add something to that list when someone adds it to the ban list.

[Peter Kasting]

On Thu, Mar 21, 2019 at 9:24 AM Daniel Bratell <bra...@opera.com> wrote:

It won't be much of an extra burden to add something to that list when someone adds it to the ban list.

So who's volunteering to update this with the current state of our ban list and test it out?  I'm not volunteering, I have too many irons in the fire on styleguide stuff.

PK

[Daniel Bratell]

An example for what could be added for std::to_string (modulo actual text).

https://chromium-review.googlesource.com/c/chromium/src/+/1535872

Not that I fully knew what to recommend instead of std::to_string. Just saying "don't do this" seems less then helpful.

[Gabriel Charette]

Le jeu. 21 mars 2019 12 h 24, Daniel Bratell <bra...@opera.com> a écrit :

On Thu, 21 Mar 2019 16:40:51 +0100, <dan...@chromium.org> wrote:

On Thu, Mar 21, 2019 at 11:38 AM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Thu, Mar 21, 2019 at 3:30 AM Daniel Bratell <bra...@opera.com> wrote:

On Wed, 20 Mar 2019 21:07:41 +0100, 'Peter Kasting' via cxx <c...@chromium.org> wrote:

On Wed, Mar 20, 2019 at 11:44 AM Gabriel Charette <g...@chromium.org> wrote:

On Fri, Mar 15, 2019 at 1:05 PM 'Peter Kasting' via cxx <c...@chromium.org> wrote:

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.

I saw std::to_string for the first in a review yesterday. Looked up our guidelines and didn't object since we don't ban it

We do ban it; it's in "C++11 Standard Library Features To Be Discussed", which says "The following C++ library features are currently disallowed."  All TBD features are banned until approved.  It would be good if you could let your reviewee know that they need to use base::NumberToString() instead.

 

and it looks like there are 100s of existing callers in our codebase.

In fairness, more than half of those are third_party and v8 code.  That leaves about 200 cases, which is higher than I'd hope for a banned feature but not very high in the grand scheme of things for a feature which has no presubmit check against it.  Heck, std::shared_ptr appears 63 times in those same directories and that's much more obviously banned.

Should probably accompany bans with presubmit warnings/errors just to avoid accidents, and to trigger official discussions rather than people disregarding the rules. Any downside to expanding the exiting ban lists in PRESUBMIT.py with std:c functions?

My only reasons would be:

• PRESUBMIT is already very slow and it would be nice not to make it slower.  Can we make this a Tricium check or something?

It is mostly slow because of the git cl format check, that is IIRC more than 70% of the time. I doubt adding these sorts of things would be noticable. 

Strong +1. This isn't the first time I hear resentment towards adding new presubmits based on the argument that it's already slow. The vast majority of the overhead is in git commands (owners check are also notoriously slow -- at least on Windows). Preventing extra fast python checks on the textual diffs is only hurting us.

• As someone who's spent a lot of time in the past trying repeatedly to get a presubmit check not to have false positives, it'd be nice not to obligate people who propose style changes to do this work unless it's both trivial and robust.

Not having a presubmit makes me doubt the importance of a Chromium ban. Typically there's a reason beyond style to ban a C++feature and if that reason is valid we want to be strict about usage. So I do think we should do everything we can do add presubmits for Chromium banned usage rules.

A well written presubmit test could have a list of banned symbols and then we need only add to the list in the future.

There is already such a list (actually several lists, one for java, one for mm, one for cpp): https://cs.chromium.org/chromium/src/PRESUBMIT.py?q=PRESUBMIT.p&sq=package:chromium&g=0&l=210

+1 to using that

It bans things like NULL, #pragma comment, FRIEND_TEST, std::regex, 

They can be either warnings or errors. I think it's important that errors have no false positives because using try=nope in cq is awful.

I'd vote for errors for style rules, there shouldn't be a reason to diverge and there's always an alternative. The problem with warnings is that they can be skipped without any visible remainder post upload for the reviewer.

Maybe warning-only while existing usage is removed to avoid errors in unrelated refactoring of surrounding code.

[dan...@chromium.org]

Yes please don't introduce errors on code that already exists, that is super painful. :)

[Daniel Bratell]

std::to_string is now a presubmit warning

std::shared_ptr is now a presubmit error

A requested "why stop here?" followup that is not yet reviewed  ( https://chromium-review.googlesource.com/c/chromium/src/+/1539040 ) adds most of the rest of of "banned" from C++11/14. The only thing on the list where I saw non-trivial amounts of usage outside third_party was for "long long". I left it as an error since I assumed it would be a trivial mechanical change in case someone innocent triggered that error.

I did note that the documentation for std::bind says "use base::Bind" and if you use base::Bind you will be told to "use base::BindOnce or base::BindRepeating". A merry dance.

[Gabriel Charette]

Great, thanks Daniel!

[Daniel Bratell]

On Sat, 23 Mar 2019 13:59:46 +0100, Gabriel Charette <g...@chromium.org> wrote:

It bans things like NULL, #pragma comment, FRIEND_TEST, std::regex, 

They can be either warnings or errors. I think it's important that errors have no false positives because using try=nope in cq is awful.

I'd vote for errors for style rules, there shouldn't be a reason to diverge and there's always an alternative. The problem with warnings is that they can be skipped without any visible remainder post upload for the reviewer.

Maybe warning-only while existing usage is removed to avoid errors in unrelated refactoring of surrounding code.

Landed the presubmit errors and warnings for the rest of the banned functions now and I hope that when everyone now knows how easy it is, we can keep adding (and removing) presubmit checks as the the set of banned functions changes.

A couple of remaining problems (discovered in the process)

The presubmit check for NULL seems broken. https://crbug.com/945971

std::function has too many uses so it could only be a warning.

Same with std::to_string.

Presubmit warnings can be ignored, but that shouldn't become a habit. Unfortunately I think the base::Bind warning might have trained people into ignoring presubmit warnings. Just too common to have that warn when you do an unrelated change. I count 11,915 "base::Bind(" in the code...

/Daniel

It won't be much of an extra burden to add something to that list when someone adds it to the ban list.

/Daniel

--

/* Opera Software, Linköping, Sweden: CET (UTC+1) */

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To post to this group, send email to c...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAJTZ7LL4iOjV6BikH%3D9j5L7SK7gRkJhpj4QRp-K_zU32p31xhA%40mail.gmail.com.

[Peter Kasting]

On Fri, Mar 15, 2019 at 10:04 AM Peter Kasting <pkas...@google.com> wrote:

Summary so far:

• Ban std::weak_ptr since it's useless without std::shared_ptr
• Ban string/number conversions (I think the exceptions argument is compelling here).  Note that in C++17 std::from_chars()/to_chars() might be of interest to us, as they are locale-independent, non-throwing, and designed for high speed.
• No discussion of UTF-xx types or aligned storage, so presumably we allow these
  

These changes have now been landed.

PK