Skip to content

Conversation

@canova
Copy link
Contributor

@canova canova commented Jul 23, 2017

This will help me to reduce the code size of my work in servo.


This change is Reviewable

@canova
Copy link
Contributor Author

canova commented Jul 23, 2017

r? @SimonSapin or anyone?

@SimonSapin
Copy link
Member

Where would you use this? (0, 0) marks the start of the input. Is this desired? Wouldn’t an obviously invalid location like (u32::MAX, u32::MAX) be preferable? Does it need to be the Default trait rather than some constructor?

@emilio
Copy link
Member

emilio commented Jul 23, 2017

Yeah, I don't think (0, 0) it's a good default... Why is this needed?

@canova
Copy link
Contributor Author

canova commented Jul 23, 2017

I'm implementing @font-feature-values rule and I need this to be able to derive Default for FontFeatureValuesRule struct. That means empty FontFeatureValuesRule, I think we don't need to set MAX value or other values for this because these values will be overwritten at some point. Also FontFaceRuleData has something similar. This method can also be changed with Default derive after this.
I'm okay to change this with a constructor named zero or something. But that will make deriving Default for FontFeatureValuesRule or FontFaceRuleData not possible.

@emilio
Copy link
Member

emilio commented Jul 23, 2017

I don't think empty() should be implemented using Default, conceptually... But no strong opinion I guess. This seems somewhat easy to misuse, that's all.

So I'm fine with whatever @SimonSapin decides.

@SimonSapin
Copy link
Member

That empty constructor should take a SourceLocation as a parameter.

Using Default would allow writing shorter code, but I think in this case it is wrong conceptually. Please write the longer code, sorry :) (Like for FontFaceRuleData::empty, the repetitive parts can probably be generated by a macro.)

@canova
Copy link
Contributor Author

canova commented Jul 23, 2017

Oh okay, I just wanted to shorten the code but if you think it can be misused, we can close this.

@canova canova closed this Jul 23, 2017
@canova canova deleted the derive branch July 23, 2017 23:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants