Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

[core] Drop LayerType #13624

Merged
merged 1 commit into from
Jan 4, 2019
Merged

[core] Drop LayerType #13624

merged 1 commit into from
Jan 4, 2019

Conversation

pozdnyakov
Copy link
Contributor

Drop LayerType and its remaining usages.
The generic code should be layer type agnostic.

Copy link
Contributor

@alexshalamov alexshalamov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm with nit 👍

@@ -16,6 +16,7 @@ class BucketParameters;
class CircleBucket : public Bucket {
public:
CircleBucket(const BucketParameters&, const std::vector<const RenderLayer*>&);
~CircleBucket() override;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: declare as final? Also, for the rest of buckets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 however, making the whole class final makes it more readable

// Implementations of this class check at least that this bucket has
// the same layer type with the given layer, but extra checks are also
// possible.
virtual bool fitsLayer(const style::Layer::Impl&) const { return true; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supports / accepts? Layer is parameter, Bucket.supports(Layer) looks bit better, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, thanks!

Drop LayerType and its remaining usages.
The generic code should be layer type agnostic.
@pozdnyakov pozdnyakov merged commit c621b81 into master Jan 4, 2019
@pozdnyakov pozdnyakov deleted the mikhail_drop_layer_type branch January 4, 2019 09:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants