C++17 Proposal: Allow inline variables

[Peter Kasting]

I propose to allow C++17 inline variables.

Basically, this lets us specify the value of class-static constexprs in a header file, even when the types involved are not integral.  This will be useful everywhere we either declare a variable in the header and define it in the .cc, or (if I'm not mistaken) define a variable in the .h, but then put a "definition point" in the .cc for the linker.

TBH, people will likely depend on this behavior in short order without realizing it, so allowing it sooner rather than later seems better.

PK

[K. Moon]

I definitely think this should be one of the first, if not the first, because of the reasoning that it's going to get used without even thinking about it anyway.

Will each approved proposal come along with a PSA about proper usage?

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFBACWbp58Bb1Pqs3-vGq8e_kBGRrgfSwLoLcLWjoLD6Hw%40mail.gmail.com.

[Daniel Cheng]

Are there any gotchas with inline variables and component builds? We'd probably get a unique instance of each inline variable per component right?

Daniel

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-5unDKweP9emrLXyMb%2BwveJp33xH550gH7y5cCnh_uHpQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Dec 24, 2021 at 9:30 AM Daniel Cheng <dch...@chromium.org> wrote:

Are there any gotchas with inline variables and component builds? We'd probably get a unique instance of each inline variable per component right?

Honestly I'm not really sure how inline variables that aren't used as constants work.  I'm trying to read cppreference.com on the subject, but it is not enlightening me.  I mostly just want to have "static constexpr T" be the officially-approved way to do class-scope constants :(.

PK

[Will Cassella]

Got linked to this thread from a CL fixing an instance of this, definitely agree that people will start using without even thinking about it, since I remember being initially shocked that this wasn't how `static constexpr` worked before.

For the case of non-constexpr `static inline` variables, it seems the way this works is by generating a "weak" definition of the symbol in each translation unit that gets coalesced to a single definition during linking. I'd guess that if you're using shared objects with this you'll end up with a unique address for that variable in each component.

I think a PSA should include the fact that the `inline` keyword is implied when the variable is declared as `static consexpr`, and we probably shouldn't let people use `static inline`.

I whipped up a simple test of what this all generates for anyone who's interested: https://github.com/willcassella/static-constexpr-test

[Will Cassella]

I updated the test to also check shared objects, and it looks like the dynamic linker can coalesce weak symbols too, which is kind of neat. It almost certainly has some binary size overhead, and I have no idea how portable this behavior is, so still don't think I'd recommend using `static inline`. Definitely +1 for `static constexpr` though.

[K. Moon]

Windows apparently requires the use of dllexport/dllimport for inline variables in DLLs (with the dllexport defined exactly once), which makes inline less useful:

https://developercommunity.visualstudio.com/t/inline-variables-are-not-useful-in-dll-due-to-c249/1262233

[Jeremy Roman]

On Wed, Dec 29, 2021 at 10:34 AM K. Moon <km...@chromium.org> wrote:

Windows apparently requires the use of dllexport/dllimport for inline variables in DLLs (with the dllexport defined exactly once), which makes inline less useful:

https://developercommunity.visualstudio.com/t/inline-variables-are-not-useful-in-dll-due-to-c249/1262233

On Tue, Dec 28, 2021, 2:13 PM Will Cassella <cas...@chromium.org> wrote:

I updated the test to also check shared objects, and it looks like the dynamic linker can coalesce weak symbols too, which is kind of neat. It almost certainly has some binary size overhead, and I have no idea how portable this behavior is, so still don't think I'd recommend using `static inline`. Definitely +1 for `static constexpr` though.

Why are static inline and static constexpr different in this regard? It's problematic for even constexpr variables to be ODR-used and the symbols not to be coalesced, because having the same variable have two different addresses is an ODR violation and thus undefined behavior.

It would be good to understand how this works in component builds across the dynamic linkers on Win, Mac and Linux (and I suppose also Fuchsia).

 

On Tuesday, December 28, 2021 at 1:18:08 PM UTC-8 Will Cassella wrote:

Got linked to this thread from a CL fixing an instance of this, definitely agree that people will start using without even thinking about it, since I remember being initially shocked that this wasn't how `static constexpr` worked before.

For the case of non-constexpr `static inline` variables, it seems the way this works is by generating a "weak" definition of the symbol in each translation unit that gets coalesced to a single definition during linking. I'd guess that if you're using shared objects with this you'll end up with a unique address for that variable in each component.

Don't constexpr ones work the same way? They still need to have a unique address even though their values are compile-time constants (they can still be ODR-used).

I think a PSA should include the fact that the `inline` keyword is implied when the variable is declared as `static consexpr`, and we probably shouldn't let people use `static inline`.

I whipped up a simple test of what this all generates for anyone who's interested: https://github.com/willcassella/static-constexpr-test

On Friday, December 24, 2021 at 10:35:59 AM UTC-8 Peter Kasting wrote:

On Fri, Dec 24, 2021 at 9:30 AM Daniel Cheng <dch...@chromium.org> wrote:

Are there any gotchas with inline variables and component builds? We'd probably get a unique instance of each inline variable per component right?

Honestly I'm not really sure how inline variables that aren't used as constants work.  I'm trying to read cppreference.com on the subject, but it is not enlightening me.  I mostly just want to have "static constexpr T" be the officially-approved way to do class-scope constants :(.

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4xAA2vZEYapZ6puyQmfcJMGPLib9SUF%2BtCN0cJ9OANcg%40mail.gmail.com.

[Roland Bock]

On Wed, Dec 29, 2021 at 5:50 PM Jeremy Roman <jbr...@chromium.org> wrote:

On Wed, Dec 29, 2021 at 10:34 AM K. Moon <km...@chromium.org> wrote:

Windows apparently requires the use of dllexport/dllimport for inline variables in DLLs (with the dllexport defined exactly once), which makes inline less useful:

https://developercommunity.visualstudio.com/t/inline-variables-are-not-useful-in-dll-due-to-c249/1262233

The answer from MS says:

"you can remove the ‘__declspec(dllimport)’ specifier from the definition to fix the issue"

Why would the use of dllexport/dllimport be required?

 

On Tue, Dec 28, 2021, 2:13 PM Will Cassella <cas...@chromium.org> wrote:

I updated the test to also check shared objects, and it looks like the dynamic linker can coalesce weak symbols too, which is kind of neat. It almost certainly has some binary size overhead, and I have no idea how portable this behavior is, so still don't think I'd recommend using `static inline`. Definitely +1 for `static constexpr` though.

Why are static inline and static constexpr different in this regard? It's problematic for even constexpr variables to be ODR-used and the symbols not to be coalesced, because having the same variable have two different addresses is an ODR violation and thus undefined behavior.

Note that we already have tons of static constexpr variables.

https://source.chromium.org/search?q=%22static%20constexpr%22&sq=&ss=chromium%2Fchromium%2Fsrc

"A constexpr specifier used in a function or static data member (since C++17) declaration implies inline."

(https://en.cppreference.com/w/cpp/language/constexpr)

Thus, we have static constexpr inline variables.

"[...] For an inline function or inline variable (since C++17), a definition is required in every translation unit where it is odr-used. [...]"
https://en.cppreference.com/w/cpp/language/definition

The way I read this: As long as you don't mess with declaring those inline functions/variables in other places you should be fine.

 

It would be good to understand how this works in component builds across the dynamic linkers on Win, Mac and Linux (and I suppose also Fuchsia).

 

On Tuesday, December 28, 2021 at 1:18:08 PM UTC-8 Will Cassella wrote:

Got linked to this thread from a CL fixing an instance of this, definitely agree that people will start using without even thinking about it, since I remember being initially shocked that this wasn't how `static constexpr` worked before.

For the case of non-constexpr `static inline` variables, it seems the way this works is by generating a "weak" definition of the symbol in each translation unit that gets coalesced to a single definition during linking. I'd guess that if you're using shared objects with this you'll end up with a unique address for that variable in each component.

Don't constexpr ones work the same way? They still need to have a unique address even though their values are compile-time constants (they can still be ODR-used).

I think a PSA should include the fact that the `inline` keyword is implied when the variable is declared as `static consexpr`, and we probably shouldn't let people use `static inline`.

I whipped up a simple test of what this all generates for anyone who's interested: https://github.com/willcassella/static-constexpr-test

On Friday, December 24, 2021 at 10:35:59 AM UTC-8 Peter Kasting wrote:

On Fri, Dec 24, 2021 at 9:30 AM Daniel Cheng <dch...@chromium.org> wrote:

Are there any gotchas with inline variables and component builds? We'd probably get a unique instance of each inline variable per component right?

Honestly I'm not really sure how inline variables that aren't used as constants work.  I'm trying to read cppreference.com on the subject, but it is not enlightening me.  I mostly just want to have "static constexpr T" be the officially-approved way to do class-scope constants :(.

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACwGi-4xAA2vZEYapZ6puyQmfcJMGPLib9SUF%2BtCN0cJ9OANcg%40mail.gmail.com.

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CACuR13dN7aEMnxU6taJoag2H5JSWQv1qTTGcmTYrOjdvbNy%3DTQ%40mail.gmail.com.

[K. Moon]

On Wed, Dec 29, 2021 at 9:45 AM Roland Bock <rb...@google.com> wrote:

On Wed, Dec 29, 2021 at 5:50 PM Jeremy Roman <jbr...@chromium.org> wrote:

On Wed, Dec 29, 2021 at 10:34 AM K. Moon <km...@chromium.org> wrote:

Windows apparently requires the use of dllexport/dllimport for inline variables in DLLs (with the dllexport defined exactly once), which makes inline less useful:

https://developercommunity.visualstudio.com/t/inline-variables-are-not-useful-in-dll-due-to-c249/1262233

The answer from MS says:

"you can remove the ‘__declspec(dllimport)’ specifier from the definition to fix the issue"

Why would the use of dllexport/dllimport be required?

The response is saying that you can't do "__declspec(dllimport) inline int foo = 42", you must do "__declspec(dllimport) inline int foo". It's not saying the dllimport/dllexport aren't needed; otherwise, you would get an ODR violation instead, because each DLL gets its own copy of the definition.

[K. Moon]

This writeup is interesting, as it directly discusses what Chrome does with inline functions (which existed pre-C++17):

https://blog.llvm.org/2018/11/30-faster-windows-builds-with-clang-cl_14.html

[Peter Kasting]

On Wed, Dec 29, 2021 at 10:01 AM K. Moon <km...@chromium.org> wrote:

This writeup is interesting, as it directly discusses what Chrome does with inline functions (which existed pre-C++17):

https://blog.llvm.org/2018/11/30-faster-windows-builds-with-clang-cl_14.html

Is it correct to have the takeaway here be "having different addresses for inline objects in different modules is potentially a problem, but in practice doesn't seem to be common enough to worry about"?

If so -- and since non-component builds are what we ship -- I'm somewhat inclined to just blanket-allow these despite the theoretical hazard.

I don't know what we could really do about it anyway.  C++17 deprecates the declaration-in-.cc-file for class-static constexprs, and I don't think wecan prevent people from relying on these.  I guess we could stop supporting the component build, but that seems like a big hammer.

PK

[K. Moon]

The documentation at https://clang.llvm.org/docs/UsersManual.html#the-zc-dllexportinlines-option goes into some of the subtle problems that can occur. Essentially, I think the problem cases are (1) if you want to mutate the inline variable (I would ban mutable inline variables), and (2) if you want to use a unique address (such as in a registration mechanism). If you just want an easy way to define a constant, and you're aware not to do those two things, I think it should be fine.

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFCgvO1OYEkwNpDUqcJtGkNY8UgCOXNubGsjvz_k_GVPcQ%40mail.gmail.com.

[Jeremy Roman]

Hmm, if compiler experts (Nico, Hans, etc) think it's reasonable I could live with this additional language non-conformance, especially since it doesn't affect the official build. It's unlikely to be much worse than the alternatives we're using today, which have their own drawbacks.

One example of something where this could be a concrete issue is blink::Supplementable: each supplement has a static const char[] which is the "supplement name". These are used to look up in a hash map keyed by the supplement name, hashed by its address in memory (not its contents). If someone in the future went and rewrote a supplement to use read

  static constexpr kSupplementName[] = "Foo";

Then in different DLLs where these had different definitions, this hash table would not work correctly in component builds. base::SupportsUserData works similarly. It is essential that if such cases not move to a world where they would have different addresses, even if we're otherwise okay with this non-conformance and the resulting ODR violations, on the basis of advice from compiler experts that this is the only UD we can expect to result.

[Will Cassella]

Why are static inline and static constexpr different in this regard?

Good question, I hadn't actually considered that they might not be, since I didn't consider testing what happens if you take the address of a `static consexpr` variable. Neither clang nor GCC treat a by-value reference to a `static constexpr` variable as a real reference to that variable even in non-constant contexts in debug builds, so the only way to test what happens is to take the address.

If we look at the CL I wrote we can compare how references get generated with or without definitions of those constants in `html_media_element.cc` (libblink_core.so) if we add a `printf("%p\n", &HTMLMediaElement::kMaxPlaybackRate)` statement to `audio_context.cc` (libblink_modules.so), I made a spreadsheet showing the results. It seems like the takeaway is that with or without a definition of `static constexpr` variables in a TU, the result is pretty much the same once you've enabled C++17. If you want to go back to the old behavior you should make the variable `static const`. Still no idea how this works on mac or Windows though.

[Daniel Cheng]

The problem of multiple global definitions isn't purely unique to inline variables. We've had this problem in the past with things like //mojo, where some core //mojo code was being built as a source set, and then linked into multiple components.

I think we probably want to disallow taking the address of inline variables, mutable or not. The possibility of weird breakage here is one of the reasons we never landed a base::any implementation, and is also the reason we do not use absl::any. The component build already breaks in strange ways, and it'd be unfortunate to introduce more.

As for mutable inline variables, would we lose a lot from disallowing them altogether?

Daniel

[Honglin Yu]

On Thu, 30 Dec 2021, 8:17 am Daniel Cheng, <dch...@chromium.org> wrote:

The problem of multiple global definitions isn't purely unique to inline variables. We've had this problem in the past with things like //mojo, where some core //mojo code was being built as a source set, and then linked into multiple components.

Just feel that there may be a small difference between putting global defs in headers  v.s. in source files: headers are intended to be included by others, especially for the headers in public folders, whereas for source files, there is no such "open-for-linking" implication. This may confuse people. 

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

[James Cook]

What's the consensus here?

Ban mutable inline variables, because of component build?

What about inline constexpr?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFstQzPUo%2B6A03dLXkqTONeoCW0%2BgJCk%2BJN-94Cf3yvV-%2B86yQ%40mail.gmail.com.

[Roland McGrath]

C++17 makes `static constexpr` and namespace-scope `constexpr` variables `inline` implicitly.  So if you have `constexpr` you have `inline constexpr`, whether you formally allow it or not.  I don't think there's any meaningful way to ban `inline constexpr` unless `constexpr` variables are banned altogether (which would be much worse certainly).  So it makes sense to be explicit that `inline constexpr` is fine and explain the important distinctions between it and the non-`constexpr` case.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAKLzqjE_bbZVVhV9eJaJzqJtH3X4EvkO%2BO3RSXAkK2QA-5myjQ%40mail.gmail.com.

[Daniel Cheng]

We can still enforce certain restrictions with the clang plugins, such as disallowing taking the address of an inline variable.

Daniel

[Peter Kasting]

On Fri, Jan 7, 2022 at 11:22 AM Daniel Cheng <dch...@chromium.org> wrote:

We can still enforce certain restrictions with the clang plugins, such as disallowing taking the address of an inline variable.

Do such plugins exist?  I'm kinda uncomfortable blocking this on us writing one.

PK 

[Peter Kasting]

Proposal:

Allow inline variables.  Write in the guidance that mutating them or taking the address of them is banned since doing so will break the component build.  If there exists a clang plugin to enforce this already (I doubt it?) enable it, otherwise Don't Care.

PK

[K. Moon]

+1

[James Cook]

+1

[Daniel Cheng]

Yes, we already have a clang plugin as part of the build: https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/plugins/FindBadConstructsConsumer.cpp. I think we should try to add this preemptively sooner rather than later.

Daniel

[Honglin Yu]

+1 after disallowing mutating and addressing.

[dan...@chromium.org]

I think it's reasonable to block this on writing the clang plugin bits that will prevent breaking the component build. Doing so is not hard just takes a bit of effort/time, and it's important to avoid very confusing bugs later.

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFstQzNzVEyLRobq%2B-mUt0fJ%3DLxdncwQhPaCRBoLRbu0B-6goQ%40mail.gmail.com.

[K. Moon]

I think one of the tensions here is the constexpr case, which already became inline when the compiler flag was flipped to C++17. So this is already the status quo; the best time to have done this enforcement was before the C++17 flip.

If someone is volunteering to do this as a fast follow, I think that would be a good compromise, but if nobody gets around to it for weeks or months, I don't think that makes sense.

[Will Cassella]

I was thinking about how you might update the clang plugin to address this (hah), and while you could probably catch simple cases like `&kInlineStatic` (by visiting `UnaryOp` expressions in the AST where `op == UO_AddrOf`, and then checking the decl of the argument), it would be much more difficult to catch cases like `&std::cref(kStatic).get()`. I don't think preventing implicit conversion of an inline constexpr variable to a const lvalue reference would work, since there's certainly legitimate uses for that.

Another route could be to require a wrapper type for inline constants like this, but then you would have to update the plugin to enforce use of that and it would be more cumbersome in cases where you want to plug that constant into a template argument, for example.

So I don't know if the addressing problem in component builds can be 100% prevented with a plugin. I think we should allow inline constexpr variables, but note that if you expect them to have a consistent address across modules then you're just asking for trouble.

One heuristic we could use is to catch if you try to define storage for an inline constexpr variable in a TU, since that might be a case where people are expecting the address to be consistent, and it has no real effect on the build with C++17 now anyway.

[dan...@chromium.org]

On Tue, Jan 11, 2022 at 12:56 PM Will Cassella <cas...@chromium.org> wrote:

I was thinking about how you might update the clang plugin to address this (hah), and while you could probably catch simple cases like `&kInlineStatic` (by visiting `UnaryOp` expressions in the AST where `op == UO_AddrOf`, and then checking the decl of the argument), it would be much more difficult to catch cases like `&std::cref(kStatic).get()`. I don't think preventing implicit conversion of an inline constexpr variable to a const lvalue reference would work, since there's certainly legitimate uses for that.

Unless you know something special, like that the type you're being passed is always heap allocated, storing the address of a reference is very problematic. You can be storing an address of a local variable beyond it's lifetime.

So while there are valid uses to taking a reference to a constexpr inline, and you can indeed then take an address of that, I think that handling the AddrOf case would be sufficient to avoid the worst footguns that are really new to this feature. Storing pointers from references overlaps greatly with C++ lifetime issues. If we could verify lifetimes we could probably verify ODR violations of this type at the same time, but we can't really verify either one if we can't verify the other, I think?

I would consider this check to be enough, anyway, along with a note like has been mentioned explaining the dangers.

Another route could be to require a wrapper type for inline constants like this, but then you would have to update the plugin to enforce use of that and it would be more cumbersome in cases where you want to plug that constant into a template argument, for example.

So I don't know if the addressing problem in component builds can be 100% prevented with a plugin. I think we should allow inline constexpr variables, but note that if you expect them to have a consistent address across modules then you're just asking for trouble.

One heuristic we could use is to catch if you try to define storage for an inline constexpr variable in a TU, since that might be a case where people are expecting the address to be consistent, and it has no real effect on the build with C++17 now anyway.

I think this would be a good secondary thing to look for, along with the AddrOf check above.

 

On Tuesday, January 11, 2022 at 8:15:59 AM UTC-8 km...@chromium.org wrote:

I think one of the tensions here is the constexpr case, which already became inline when the compiler flag was flipped to C++17. So this is already the status quo; the best time to have done this enforcement was before the C++17 flip.

If someone is volunteering to do this as a fast follow, I think that would be a good compromise, but if nobody gets around to it for weeks or months, I don't think that makes sense.

On Tue, Jan 11, 2022 at 6:42 AM <dan...@chromium.org> wrote:

I think it's reasonable to block this on writing the clang plugin bits that will prevent breaking the component build. Doing so is not hard just takes a bit of effort/time, and it's important to avoid very confusing bugs later.

On Mon, Jan 10, 2022 at 3:54 PM 'Honglin Yu' via cxx <c...@chromium.org> wrote:

+1 after disallowing mutating and addressing.

On Tue, Jan 11, 2022 at 5:52 AM Daniel Cheng <dch...@chromium.org> wrote:

Yes, we already have a clang plugin as part of the build: https://source.chromium.org/chromium/chromium/src/+/main:tools/clang/plugins/FindBadConstructsConsumer.cpp. I think we should try to add this preemptively sooner rather than later.

Daniel

On Fri, 7 Jan 2022 at 11:55, Peter Kasting <pkas...@google.com> wrote:

On Fri, Jan 7, 2022 at 11:22 AM Daniel Cheng <dch...@chromium.org> wrote:

We can still enforce certain restrictions with the clang plugins, such as disallowing taking the address of an inline variable.

Do such plugins exist?  I'm kinda uncomfortable blocking this on us writing one.

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFstQzNzVEyLRobq%2B-mUt0fJ%3DLxdncwQhPaCRBoLRbu0B-6goQ%40mail.gmail.com.

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/e2d77cc1-9cac-4e8c-853c-d8179324e234n%40chromium.org.

[Will Cassella]

I would consider this check to be enough, anyway, along with a note like has been mentioned explaining the dangers.

Makes sense, I'll take a crack at writing a CL for this.

I think this would be a good secondary thing to look for, along with the AddrOf check above. 

Ditto for this, though I'd probably do it as a separate CL/plugin feature since it would be a different code path anyway, and there may be existing cases of this that will have to be fixed.

[Bruce Dawson]

If I'm following this correctly then we are currently compiling as C++17 on Windows which means that constexpr variables in header files should be folded together, at least in non-component builds, with some possible exceptions.

A bug related to a constexpr variable in a header file getting multiple definitions (crbug.com/1269955) popped up for me yesterday. I just double-checked with today's 32-bit canary and it shows that there are still lots of duplicates. I thought it might be because the definition was in an anonymous namespace so I moved kOrderIndexShiftout of all namespaces and ShowGlobals still says we have a dozen copies of this array.

Does the constexpr==inline variable logic only work for scalar variables perhaps?

[dan...@chromium.org]

On Thu, Jan 13, 2022 at 12:21 AM Bruce Dawson <bruce...@google.com> wrote:

If I'm following this correctly then we are currently compiling as C++17 on Windows which means that constexpr variables in header files should be folded together, at least in non-component builds, with some possible exceptions.

A bug related to a constexpr variable in a header file getting multiple definitions (crbug.com/1269955) popped up for me yesterday. I just double-checked with today's 32-bit canary and it shows that there are still lots of duplicates. I thought it might be because the definition was in an anonymous namespace so I moved kOrderIndexShiftout of all namespaces and ShowGlobals still says we have a dozen copies of this array.

Does the constexpr==inline variable logic only work for scalar variables perhaps?

A constexpr specifier used in a function or static data member (since C++17) declaration implies inline.

https://en.cppreference.com/w/cpp/language/constexpr

A "static data member" is a static member of a class. What I read is that nothing has changed for namespace-scope variables and you should write `constexpr inline` if you want it to be inline.

https://en.cppreference.com/w/cpp/language/static

[Bruce Dawson]

Got it. I investigated a bit more and found that for this specific case I have to both explicitly mark the arrays as "constexpr inline" (as you said) and move them out of the anonymous namespace.

The compiler should probably warn if you use "constexpr inline" in an anonymous namespace because it appears that the two requests are contradictory.

This issue (constants defined in header files resulting in multiple copies) has been bugging me for over a decade and it is great to finally have an easy solution.

--

Bruce Dawson, he/him

[Bruce Dawson]

I created a CL which makes use of inline to avoid some duplicated globals, and updated C++11.md to summarize this discussion:

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

I was expecting a reduction in size of chrome.dll but I didn't see one, which seems odd. The file size and all of the section sizes are unchanged by this change. The showglobals tool says that the duplicates are gone, so I don't know how to interpret that.

[Victor Vianna]

nit: Now there are both instances of "constexpr inline" and "inline constexpr" going on in the code. Should we pick one and enforce it via `git cl format`?

[dan...@chromium.org]

They are equivalent, does it matter..?

[Victor Vianna]

Makes the code more uniform? For the Google Java styleguide (though not the Chromium one apparently), we do enforce an order for the modifier keywords.

[K. Moon]

clang-format 14 apparently has a QualifierOrder setting for this:

https://clang.llvm.org/docs/ClangFormatStyleOptions.html

There's no default, though, so I'm guessing clang-format isn't opinionated about this.

From Code Search, "inline constexpr" seems vastly (like 27:1) more popular than "constexpr inline", so I think a consistency argument at this point would go with "inline constexpr". (Victor's search is only 4:1, but it excludes third_party; I think it's useful to look at what third_party is doing in this case.)

https://abseil.io/tips/140 and https://abseil.io/tips/168 both use "inline constexpr" rather than "constexpr inline", so that might be another vote for it being a more well-known spelling.

The JLS has wording like, "If two or more (distinct) class modifiers appear in a class declaration, then it is customary, though not required, that they appear in the order consistent with that shown above in the production for ClassModifier" (https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.1.1), so I don't think the two situations are symmetrical.

[dan...@chromium.org]

On Fri, Feb 4, 2022 at 11:31 AM K. Moon <km...@chromium.org> wrote:

clang-format 14 apparently has a QualifierOrder setting for this:

https://clang.llvm.org/docs/ClangFormatStyleOptions.html

There's no default, though, so I'm guessing clang-format isn't opinionated about this.

From Code Search, "inline constexpr" seems vastly (like 27:1) more popular than "constexpr inline", so I think a consistency argument at this point would go with "inline constexpr". (Victor's search is only 4:1, but it excludes third_party; I think it's useful to look at what third_party is doing in this case.)

https://abseil.io/tips/140 and https://abseil.io/tips/168 both use "inline constexpr" rather than "constexpr inline", so that might be another vote for it being a more well-known spelling.

The JLS has wording like, "If two or more (distinct) class modifiers appear in a class declaration, then it is customary, though not required, that they appear in the order consistent with that shown above in the production for ClassModifier" (https://docs.oracle.com/javase/specs/jls/se7/html/jls-8.html#jls-8.1.1), so I don't think the two situations are symmetrical.

Right, so the Google Java guide is following the JLS. Whereas the Google C++ guide is silent. I guess trying to make the order consistent doesn't seem like a great use of time to me. If clang format ends up doing so, then sure.

[Roland McGrath]

When you're in C++17 mode, `constexpr` alone is equivalent to `inline constexpr`, so why normalize on an order instead of normalizing on just `constexpr`?

[dan...@chromium.org]

On Fri, Feb 4, 2022 at 1:17 PM Roland McGrath <mcgr...@chromium.org> wrote:

When you're in C++17 mode, `constexpr` alone is equivalent to `inline constexpr`, so why normalize on an order instead of normalizing on just `constexpr`?

That is only true for static members, right? 

[Roland McGrath]

For `static constexpr` in class scope, and for `constexpr` at namespace/global scope. 

[Roland McGrath]

Oops, sorry. I misremembered.  You're right.  It's only for class scope.

[Victor Vianna]

That's not what is suggested in our docs, I think.

[K. Moon]

In that case, I'd suggest updating the docs, since it seems inconsistent with the more common spelling, and I'd hate the other spelling to only spread because it was that way in our docs (as opposed to a more meaningful reason).

[Jan Keitel]

Hi folks,

I just stumbled across this thread as part of cleaning up some of the components/password_manager constants and I was wondering: Is there ever a reason *not* to make a `constexpr` variable definition inside a header inline?

If there isn't, should we then add a presubmit check that every constexpr variable definition in a header (and not part of a class) should be inline?

Thanks,

Jan

[Dana Jansens]

Providing a way to opt out of the check is a good idea, even if we don't think of a great reason why it might be needed. Having good defaults is important, and +1 to making inline be present be the default for non-static variables.

On Tue, Jul 18, 2023 at 2:01 AM Jan Keitel <jke...@google.com> wrote:

Agreed for declarations.

But as long as it is a constexpr definition, is there ever a case in which there's an upside to it not being inline?

On Mon, Jul 17, 2023 at 11:05 PM Joe Mason <joenot...@google.com> wrote:

Sometimes platform-specific code is implemented by having a shared copy of a header file, and linking a different .cc file implementing it on each platform. (Instead of wrapping different implementations in "#if BUILDFLAG(...)"). In that case, I could see the constant being declared in the header, and defined to a different platform-specific value in each .cc file.

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/14820417-003c-45d7-8c68-d2fd3b7fea3dn%40chromium.org.

--

Jan Keitel

Software engineer

jke...@google.com

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. 

     

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Joe Mason]

Sometimes platform-specific code is implemented by having a shared copy of a header file, and linking a different .cc file implementing it on each platform. (Instead of wrapping different implementations in "#if BUILDFLAG(...)"). In that case, I could see the constant being declared in the header, and defined to a different platform-specific value in each .cc file.

On Mon, Jul 17, 2023 at 4:34 PM 'Jan Keitel' via cxx <c...@chromium.org> wrote:

--

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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/14820417-003c-45d7-8c68-d2fd3b7fea3dn%40chromium.org.

[Jan Keitel]

Agreed for declarations.

But as long as it is a constexpr definition, is there ever a case in which there's an upside to it not being inline?

On Mon, Jul 17, 2023 at 11:05 PM Joe Mason <joenot...@google.com> wrote: