Closed Bug 1310123 Opened 8 years ago Closed 8 years ago

Avoid static const without in-class initializer in style structs

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1310123 - Move values of static constants in nsStyleStruct to the header.
58 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
Static consts have linkage and name mangling, which would break the in-tree binding across platforms.

Recently, servo/servo#13674 introduced a use of nsStyleColumn::kMaxColumnCount, which breaks Windows build for me, because MSVC's name mangling is different from that used in other platforms.

In general, having to look up memory for a constant doesn't sound very fun. We should prefer having const inline when possible.

In this case, I'm going to convert kMaxColumnCount to a macro, so that bindgen generates a "pub const" for it rather than a symbol linked to Gecko. Not using nested anonymous enum because bindgen is not very friendly to this kind of usage (which we may want to fix, though).
Would it help to convert kMaxColumnCount to be a constexpr function? I was wondering whether name mangling will transform it or not.
A static constexpr function? I don't think that helps. But I'm not sure whether a static constexpr works.

I guess we can probably use enum if we fix servo/rust-bindgen#84, so that we keep it scoped.
I know nothing about how rust-bindgen deal with something like "constexpr uint32_t MaxColumnCount() { return 1000; }", but this is what I think about in comment 1. BTW, constexpr declares on a function implies inline [1].

[1] http://en.cppreference.com/w/cpp/language/constexpr
Actually, any function declare inside class/struct body implies inline as well. That doesn't make anything different. And I don't think it is reasonable to require bindgen to convert an inline function to rust or execute it and store its result.
I mean, any function defined inside class/struct body implies inline.
Since servo/servo#13674 doesn't seem to have much progress, and this is annoying for me, I hope to land the fix first, and we can change it back to an enum constant later.
Comment on attachment 8801944 [details]
Bug 1310123 - Convert nsStyleColumn::kMaxColumnCount to a macro.

https://reviewboard.mozilla.org/r/86530/#review85622

::: layout/style/nsStyleStruct.h:3344
(Diff revision 1)
>  
> +/**
> + * This is the maximum number of columns we can process. It's used in both
> + * nsColumnSetFrame and nsRuleNode.
> + */
> +#define NS_MAX_COLUMN_COUNT 1000

Can we make it 1000U instead?
(kMaxColumnCount is currently uint32_t and I'd prefer to not make it signed)
I don't think we usually do that for macro constants: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%22%23define%5Cs%2B%5CS%2B%5Cs%2B(0x)%3F%5Cd%2Bu

I guess I'm probably fine with not landing this patch, as far as I'm the only person working on stylo on Windows. I'd like to wait for servo/servo#13674 for a bit longer and after that, convert the static constant to enum constant directly.
Attachment #8801944 - Flags: review?(mats)
Well, I think the correct type for this constant really is "unsigned integer",
not enum or anything else.
1000U is reasonable.  I'm happy using macros.
I'm not a fun of macros because they are not scoped, and could produce unexpected code in some case (although it is probably fine for this case). I'd always prefer avoiding that when possible.

I think the argument that it should be a constant makes sense. I rethink about this issue, and it seems to me it is not necessary to avoid static consts. What we should avoid is, static consts without in-class initializer.

We should move the initializer into the struct for this cases, and teach rust-bindgen to generate constant rather than extern static for it.

According to the C++ spec, static const of integer type and enum type can have in-class initializer. And a definition of the static const is not needed if it is not odr-used (nothing wants to take its address / reference).
Summary: Avoid static const in style structs → Avoid static const without in-class initializer in style structs
s/fun/fan
Attachment #8801944 - Attachment is obsolete: true
With my proposed patch in servo/rust-bindgen#126, we should be able to generate Rust constants from the C++ static constants after the change here.
Comment on attachment 8804159 [details]
Bug 1310123 - Move values of static constants in nsStyleStruct to the header.

https://reviewboard.mozilla.org/r/88282/#review87378
Attachment #8804159 - Flags: review?(mats) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9b715399e585
Move values of static constants in nsStyleStruct to the header. r=mats
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46a855ae9e4b
Move values of static constants in nsStyleStruct to the header. r=mats
Update the code and relanded.
Flags: needinfo?(xidorn+moz)
https://hg.mozilla.org/mozilla-central/rev/46a855ae9e4b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: