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

Check source usage before removal #9129

Merged
merged 4 commits into from
May 30, 2017
Merged

Conversation

ivovandongen
Copy link
Contributor

Closes #9121

I opted to log a warning instead of throwing an exception. I can see it going both ways though.

@ivovandongen ivovandongen requested a review from kkaefer May 29, 2017 11:15
@ivovandongen ivovandongen self-assigned this May 29, 2017
@ivovandongen ivovandongen added the Core The cross-platform C++ core, aka mbgl label May 29, 2017
Copy link
Contributor

@kkaefer kkaefer left a comment

Choose a reason for hiding this comment

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

I opted to log a warning instead of throwing an exception

If we made it an exception, SDKs could catch it and provide platform-native error response.

@@ -158,7 +158,30 @@ void Style::addSource(std::unique_ptr<Source> source) {
sources.emplace_back(std::move(source));
}

struct SourceIdUsageEvaluator {
std::string sourceId;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be const, or even a const ref

});

if (layerIt != layers.end()) {
Log::Warning(Event::General, "Source %s is in use, cannot remove", id.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please add quotes around the name?

@ivovandongen ivovandongen force-pushed the 9121-check-before-remove-source branch 2 times, most recently from 2dea1db to 7a67bd1 Compare May 29, 2017 12:31
@ivovandongen
Copy link
Contributor Author

@kkaefer

If we made it an exception, SDKs could catch it and provide platform-native error response.

Agreed, but we also force the SDKs to deal with it. Not sure if it's that helpful. We now have a mix of exception / log and ignore. We should align that at some point

I had to add a default case to Layer::accept btw, to make it work with non-void return values on gcc5 (otherwise, compilation fails with: "control reaches end of non-void function").

@@ -88,6 +89,8 @@ class Layer : public mbgl::util::noncopyable {
return std::forward<V>(visitor)(*as<CustomLayer>());
case LayerType::FillExtrusion:
return std::forward<V>(visitor)(*as<FillExtrusionLayer>());
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding default means that clang won't issue an "enumeration value not handled in switch" error if you add a new layer type enum but forget to add it here. Use this convention instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Although returning a nullptr is not an option here. I couldn't find a good way to return in all instantiations (eg. for void, stack based values and pointers), so I kept the runtime_error to appease gcc.


if (layerIt != layers.end()) {
Log::Warning(Event::General, "Source '%s' is in use, cannot remove", id.c_str());
return nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are SDKs prepared for removeSource returning a null pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfirebaugh Yes, a nullptr was already returned in case the source doesn't exist here. I've added tests for the Android and iOS/macos SDKs to ensure this.

@ivovandongen ivovandongen force-pushed the 9121-check-before-remove-source branch from 7a67bd1 to 9f7aad7 Compare May 30, 2017 09:07
@ivovandongen ivovandongen merged commit fb3cef6 into master May 30, 2017
@ivovandongen ivovandongen deleted the 9121-check-before-remove-source branch May 30, 2017 10:07
@tobrun tobrun mentioned this pull request Jun 21, 2017
11 tasks
@tobrun tobrun mentioned this pull request Jun 30, 2017
16 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants