Reminder: For string literals, generally prefer "extern const s[];" + out-of-line definition over "constexpr char s[] = ..."

[Nico Weber]

Hi,

for strings, we currently say

    extern const char kAboutBlankURL[];

in a .h file and then

    const char kAboutBlankURL[] = "about:blank";

in a .cc file. This is good, we should keep doing it.

I've seen a few people instead do

    constexpr char kAboutBlankURL[] = "about:blank";

in a .h file, without a .cc file. This is almost always worse: constexpr implies const implies internal linkage, meaning every translation unit that references kAboutBlankURL will now refer to its own local copy of the string, and we'd have to rely on the linker to merge all these identical strings. This is not behavior-preserving (since the standard says the different copies of the string are supposed to live at different addresses) and so it isn't done by default. We plan to explicitly tell the linker to do this merging, but we don't do that today. And making the compiler do a bunch of work that the linker then has to undo is a bit silly anyways.

There are almost no advantages to making strings constexpr, so let's keep doing what we're currently doing.

Nico

[Joe Mason]

To clarify: strings that are used only within a .cc file should be constexpr, yes?

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiELMmHUV%2BorWB6o%2B1zbQeN5w%2BPTn-VxaaPKRZrZCtdMKw%40mail.gmail.com.

[Peter Collingbourne]

(resending from chromium.org)

Hi Nico,

Would it be worth considering this (in the .h file):

constexpr const char *kAboutBlankURL = "about:blank";

With that the standard will provide no guarantees about the pointer identity of the string literal, so the linker would be allowed to merge it with other string literals (including tail merging, which might be possible with ICF on rodata, but not easily). It also of course means less code, because the string doesn't need to be declared in a .cc file. Any references to the string literal would technically be indirected via the kAboutBlankURL object containing a pointer to the string literal, but every compiler that I've tried (gcc, clang, cl) optimizes that away (including the object itself) at -O1 or greater.

One downside is that the kAboutBlankURL object would have internal linkage, which may be observable if its address is taken, but in general nobody should be taking its address anyway, so perhaps that doesn't matter. If it did matter, we may want to use the inline variable feature added to C++17:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4424.pdf

and write:

inline constexpr const char *kAboutBlankURL = "about:blank";

instead.

Peter

--

[Nico Weber]

Joe: I don't know of a downside of marking .cc-only strings as constexpr (but I don't really know an upside either).

Peter: That seems pretty subtle to me :-) Given that `extern const kFoo[]` is very widely used, reasonably well-understood, and works well, I'd be hesitant to recommend something else unless it has many advantages (or it has minor advantages and someone signs up to convert everything to the new style, I suppose).

[Joe Mason]

I believe clang-style currently rejects "constexpr kFoo*" and requires [] with constexpr. Presumably that can be changed, but also presumably it rejects it for a reason.

(I could be wrong, it's a while since I've tried.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAMGbLiFA7%2B3LsVCbnEAiFrU%3Du1bLRaRgXvzyDy098BUTPpG8-A%40mail.gmail.com.

[Brandon Tolsch]

My $0.02: if char[] is actually being used for std::string operations, constexpr isn't helpful anyway since std::string doesn't have a constexpr constructor.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAH%3DT95RvHOrY6asWiHbQ9Cmz%2BBt%2BFPxtuRvSBQuZjN97R2VgBw%40mail.gmail.com.

[Trent Apted]

On 22 June 2018 at 10:10, Brandon Tolsch <bto...@chromium.org> wrote:

My $0.02: if char[] is actually being used for std::string operations, constexpr isn't helpful anyway since std::string doesn't have a constexpr constructor.

base::StringPiece has them though :)

We could even add a templatized

template <int STRLEN_PLUS_ONE>

constexpr BasicStringPiece(const value_type (&string_literal)[N]) : ptr_(string_literal), length_(STRLEN_PLUS_ONE - 1) {}

which would work with  constexpr char kAboutBlankURL[] = "about:blank"; - but only those in a header file.... (#minor-advantage)

(or, in fact, comments in string_piece.h suggest STRING_TYPE::traits_type::length() is constexpr in C++17, so maybe we don't even need that template)

Concretely, this would avoid calls to strlen being done, e.g., by base::CommandLine::HasSwitch(..) during Chrome startup.

 

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

[Mike Wittman]

On Thu, Jun 21, 2018 at 6:02 PM Trent Apted <tap...@chromium.org> wrote:

On 22 June 2018 at 10:10, Brandon Tolsch <bto...@chromium.org> wrote:

My $0.02: if char[] is actually being used for std::string operations, constexpr isn't helpful anyway since std::string doesn't have a constexpr constructor.

base::StringPiece has them though :)

We could even add a templatized

template <int STRLEN_PLUS_ONE>

constexpr BasicStringPiece(const value_type (&string_literal)[N]) : ptr_(string_literal), length_(STRLEN_PLUS_ONE - 1) {}

which would work with  constexpr char kAboutBlankURL[] = "about:blank"; - but only those in a header file.... (#minor-advantage)

(or, in fact, comments in string_piece.h suggest STRING_TYPE::traits_type::length() is constexpr in C++17, so maybe we don't even need that template)

Concretely, this would avoid calls to strlen being done, e.g., by base::CommandLine::HasSwitch(..) during Chrome startup.

For context we spend on average <6ms of the first 30 seconds of startup executing CommandLine::HasSwitch() on the UI thread on Windows, not all of which is in strlen. Example sampling profiler link [Google only unfortunately] from a recent dev. Which would appear to corroborate that this would confer only a minor advantage. :)

 

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAGrfxdHD2Z9mA%3D4NYw9fMw_SutgSANPyyS%2Bu%2BPa%3Dbz50%3DxzOA%40mail.gmail.com.

[Peter Kasting]

I think the advantage of a simple rule saying "use constexpr for compile-time constants" is important.  I'm not a big fan of having to remember exceptions for various types.

It seems to me like C++17 inline variables might address this (and several similar cases).  If we were in a world where we had those, would you be happy with people declaring constexpr string constants in headers?  If so, my suggestion would be "let's not worry about this too much either way until C++17 support is possible for Chrome [hopefully some time reasonably soon], then we'll try to allow inline variables quickly."

PK

[Nico Weber]

Do we have a rule like this? I can't see it on http://chromium-cpp.appspot.com/, but maybe I'm looking in the wrong place. Or are you saying we should strive to have a rule like this? "int constants in headers, string constants out of line" is what I thought we had been doing since chrome existed.

I agree that having such a rule would be nice. But since introducing that rule right now both has practical drawbacks and is inconsistent with almost all existing code (again, as far as I know), I'm not sure if we shouldn't wait with changing rules until the new rule is actually better and not better-in-some-regards-worse-in-others.

[Peter Kasting]

On Fri, Jun 22, 2018 at 1:35 PM Nico Weber <tha...@chromium.org> wrote:

Do we have a rule like this?

Yes, in the Google style guide; see the first sentence of http://google.github.io/styleguide/cppguide.html#Use_of_constexpr .

 

"int constants in headers, string constants out of line" is what I thought we had been doing since chrome existed.

I'm not aware of that being a consistent common practice.  "Random stuff in headers or .cc files or functions within .cc files" is all I can detect :)

I'm still curious whether, in a world where we had C++17 inline variables, your concerns would be addressed, or if there are still problems even then.

PK

[Nico Weber]

On Fri, Jun 22, 2018 at 4:41 PM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Jun 22, 2018 at 1:35 PM Nico Weber <tha...@chromium.org> wrote:

Do we have a rule like this?

Yes, in the Google style guide; see the first sentence of http://google.github.io/styleguide/cppguide.html#Use_of_constexpr .

 

"int constants in headers, string constants out of line" is what I thought we had been doing since chrome existed.

Ah, thanks for pointing that out. That's unfortunate :-/

 

I'm not aware of that being a consistent common practice.  "Random stuff in headers or .cc files or functions within .cc files" is all I can detect :)

constexpr char kFoo[] in headers has 53 hits:

https://cs.chromium.org/search/?q=constexpr%5C+char.*%5C%5B+file:%5C.h&sq=package:chromium&type=cs

extern const char kFoo[] has 1082:

https://cs.chromium.org/search/?q=extern%5C+const%5C+char.*%5C%5B+file:%5C.h&sq=package:chromium&type=cs

I'm still curious whether, in a world where we had C++17 inline variables, your concerns would be addressed, or if there are still problems even then.

I'd be happy with that. (It still means that the compiler does a bunch of work that the linker needs to undo, but it's small compared to, say, inline functions where the same thing already happens.)

[Peter Kasting]

On Fri, Jun 22, 2018 at 1:53 PM Nico Weber <tha...@chromium.org> wrote:

On Fri, Jun 22, 2018 at 4:41 PM Peter Kasting <pkas...@chromium.org> wrote:

I'm not aware of that being a consistent common practice.  "Random stuff in headers or .cc files or functions within .cc files" is all I can detect :)

constexpr char kFoo[] in headers has 53 hits:

https://cs.chromium.org/search/?q=constexpr%5C+char.*%5C%5B+file:%5C.h&sq=package:chromium&type=cs

extern const char kFoo[] has 1082:

https://cs.chromium.org/search/?q=extern%5C+const%5C+char.*%5C%5B+file:%5C.h&sq=package:chromium&type=cs

Oh, sure, in terms of const vs. constexpr, most of the codebase still uses the former, for everything (strings and ints both).  I meant the more general pattern you described, that we e.g. put ints consistently in headers.

I'm still curious whether, in a world where we had C++17 inline variables, your concerns would be addressed, or if there are still problems even then.

I'd be happy with that. (It still means that the compiler does a bunch of work that the linker needs to undo, but it's small compared to, say, inline functions where the same thing already happens.)

K.  So the next question would probably be, if we are inconsistent about this now (or perhaps, if we were to consistently start using constexpr char[] literals in headers now), what's the worst that happens?  Compile and link times increase?  Binary size increases?  Basically, how urgent is it that people remember this exception?

My instinct is that it's not urgent enough for me to keep paged in to my brain (I also can never remember whether I'm supposed to declare char[] constants inside functions as static or not), but maybe I'm wrong.

PK

[Alexandr Ilin]

I want to define a string in a header file that I could process at compile-time using constexpr functions. There is not so many options, and defining a string as "constexpr char s[] = ..." is one of them. I cannot use extern with constexpr.

There is another way which is to declare a string as a static constexpr field of a class. It's been discussed here: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zu1NMS0RyrM

Which way should I prefer, given that this string will probably be not used at run-time at all?

CL for context: https://crrev.com/c/1251361

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFC2%2BpBbo%3D-0jg2Gz7H-hCQsdD0PxNe2U%3DOuHCrdUqqbMQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Nov 9, 2018 at 9:38 AM Alexandr Ilin <alex...@chromium.org> wrote:

I want to define a string in a header file that I could process at compile-time using constexpr functions. There is not so many options, and defining a string as "constexpr char s[] = ..." is one of them. I cannot use extern with constexpr.

There is another way which is to declare a string as a static constexpr field of a class. It's been discussed here: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/zu1NMS0RyrM

Which way should I prefer, given that this string will probably be not used at run-time at all?

CL for context: https://crrev.com/c/1251361

I looked at your CL, but there are a lot of files, and the CL seems to use both methods.  Are there particular cases within that CL where you're debating this?

PK 

[Scott Violet]

I requested that Alexander bring the issue up in this thread. Based on this thread it seemed the agreement was to use extern const char vs constexpr (at least in headers). Alexander's patch converts a number of headers to constexpr, which is counter to this thread. It may be constexpr is the right thing for Alexander's patch, but I wanted to make sure we were ok with that and there aren't any bad side effects (such as unnecessary duplicate in different compile units).

  -Scott

[James Cook]

For Googlers, go/totw/140 has some discussion of this. Specifically:

// In foo.h

constexpr char kSpecial[] = "special";

This creates one constant per translation unit that includes foo.h. It can result in &kSpecial having different values in different translation units. This is rare, but has apparently caused bugs. See link for details.

I don't know what effect it has on compile or link times.

James

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

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

[Peter Kasting]

On Fri, Nov 9, 2018 at 4:38 PM James Cook <jame...@chromium.org> wrote:

For Googlers, go/totw/140 has some discussion of this. Specifically:

// In foo.h

constexpr char kSpecial[] = "special";

This creates one constant per translation unit that includes foo.h. It can result in &kSpecial having different values in different translation units. This is rare, but has apparently caused bugs. See link for details.

It's worth noting that the context of this request are cases that

1. Do not take the address of the constant
2. Are "constexpr const char kFoo[]", not "constexpr char kFoo[]"; the two are meaningfully different and I can imagine the former being more amenable to not appearing in the binary

 In that context, if it's actually true that there are no symbols produced for these constants, I'm fine with this.  I'm not certain whether no symbols are produced.  Seems like onus is on the author to demonstrate that.  The claim is that the overall binary is 60kB smaller, which sounds like this is a win.

PK

[Greg Thompson]

On Sat, Nov 10, 2018 at 1:43 AM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Nov 9, 2018 at 4:38 PM James Cook <jame...@chromium.org> wrote:

For Googlers, go/totw/140 has some discussion of this. Specifically:

// In foo.h

constexpr char kSpecial[] = "special";

This creates one constant per translation unit that includes foo.h. It can result in &kSpecial having different values in different translation units. This is rare, but has apparently caused bugs. See link for details.

It's worth noting that the context of this request are cases that

1. Do not take the address of the constant
2. Are "constexpr const char kFoo[]", not "constexpr char kFoo[]"; the two are meaningfully different and I can imagine the former being more amenable to not appearing in the binary

I had thought that the second form ("constexpr char kFoo[] = "...";) meant "make kFoo a constant array holding the given terminated string and make it a compile-time constant." If this is true, it's the same as the first form ("constexpr const char kFoo[] = "...";), no? How are they different? Does the second form actually result in a char array that can be modified at runtime? Educate me! :-)

 

 In that context, if it's actually true that there are no symbols produced for these constants, I'm fine with this.  I'm not certain whether no symbols are produced.  Seems like onus is on the author to demonstrate that.  The claim is that the overall binary is 60kB smaller, which sounds like this is a win.

PK

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFBJ17HwmNYtp51qkKL5tfTKXXf%3Dk0FU5SM7uwkztVZtmQ%40mail.gmail.com.

[Peter Kasting]

On Tue, Nov 13, 2018 at 3:05 AM Greg Thompson <g...@chromium.org> wrote:

On Sat, Nov 10, 2018 at 1:43 AM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Nov 9, 2018 at 4:38 PM James Cook <jame...@chromium.org> wrote:

For Googlers, go/totw/140 has some discussion of this. Specifically:

// In foo.h

constexpr char kSpecial[] = "special";

This creates one constant per translation unit that includes foo.h. It can result in &kSpecial having different values in different translation units. This is rare, but has apparently caused bugs. See link for details.

It's worth noting that the context of this request are cases that

1. Do not take the address of the constant
2. Are "constexpr const char kFoo[]", not "constexpr char kFoo[]"; the two are meaningfully different and I can imagine the former being more amenable to not appearing in the binary

I had thought that the second form ("constexpr char kFoo[] = "...";) meant "make kFoo a constant array holding the given terminated string and make it a compile-time constant." If this is true, it's the same as the first form ("constexpr const char kFoo[] = "...";), no? How are they different? Does the second form actually result in a char array that can be modified at runtime? Educate me! :-)

I think I'm confused.  Arrays and pointers don't work the same here, and I was thinking of pointers.

These two are meaningfully different:

constexpr char* foo

constexpr const char* foo

...because the "constexpr" makes the pointer const in both cases, rather than the data pointed to.*

But array types are not assignable, so "char foo[]" is more like "char* const foo" than "char* foo".  So apparently the compiler parses "constexpr char foo[]" more like it parses "constexpr const char* const foo".

PK

*For example, this compiles:

char c = 'a';

constexpr char* const p_c = &c;

int main() {

  *p_c = 'b';

  return 0;

}

[Antoine Labour]

On Tue, Nov 13, 2018 at 10:09 AM Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Nov 13, 2018 at 3:05 AM Greg Thompson <g...@chromium.org> wrote:

On Sat, Nov 10, 2018 at 1:43 AM Peter Kasting <pkas...@chromium.org> wrote:

On Fri, Nov 9, 2018 at 4:38 PM James Cook <jame...@chromium.org> wrote:

For Googlers, go/totw/140 has some discussion of this. Specifically:

// In foo.h

constexpr char kSpecial[] = "special";

This creates one constant per translation unit that includes foo.h. It can result in &kSpecial having different values in different translation units. This is rare, but has apparently caused bugs. See link for details.

It's worth noting that the context of this request are cases that

1. Do not take the address of the constant
2. Are "constexpr const char kFoo[]", not "constexpr char kFoo[]"; the two are meaningfully different and I can imagine the former being more amenable to not appearing in the binary

I had thought that the second form ("constexpr char kFoo[] = "...";) meant "make kFoo a constant array holding the given terminated string and make it a compile-time constant." If this is true, it's the same as the first form ("constexpr const char kFoo[] = "...";), no? How are they different? Does the second form actually result in a char array that can be modified at runtime? Educate me! :-)

I think I'm confused.  Arrays and pointers don't work the same here, and I was thinking of pointers.

These two are meaningfully different:

constexpr char* foo

constexpr const char* foo

...because the "constexpr" makes the pointer const in both cases, rather than the data pointed to.*

constexpr applies to the variable, not the type. https://en.cppreference.com/w/cpp/language/constexpr

Antoine

 

But array types are not assignable, so "char foo[]" is more like "char* const foo" than "char* foo".  So apparently the compiler parses "constexpr char foo[]" more like it parses "constexpr const char* const foo".

PK

*For example, this compiles:

char c = 'a';

constexpr char* const p_c = &c;

int main() {

  *p_c = 'b';

  return 0;

}

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAAHOzFA7wqvRK0_6MLD4QbAL4%3DxQcWSFKMA8m5nqXSrwLtF_ZA%40mail.gmail.com.