Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Implement Base32Decode #12253

Merged
merged 2 commits into from
Sep 16, 2019
Merged

Implement Base32Decode #12253

merged 2 commits into from
Sep 16, 2019

Conversation

liyuqian
Copy link
Contributor

For flutter/flutter#32170

This is to enable reading back SkSL persistent cache filenames
and decode them as SkData.

Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Some nits. Not really sure why a string couldn't be used instead of a vector or chars. But lgtm otherwise.

fml/base32.cc Outdated
}
bits_length += 5;
bits = (bits << 5) | kDecodeMap[map_index];
FML_DCHECK(bits_length < 16);
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a release check or just return false from this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I encapsulated a BitConverter class that's used for both encoding and decoding for better safety and readability. This bits_length < 16 condition is supposed to be checkable at compile time. Hopefully the encapsulated class makes it clearer.

fml/base32.cc Outdated
static constexpr int kDecodeMapSize =
sizeof(kDecodeMap) / sizeof(kDecodeMap[0]);

std::pair<bool, std::vector<char>> Base32Decode(const std::string& input) {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be just a string as well? You can use std::stringstream instead of a vector below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to string. I originally wrote vector because I'm a little worried about char '\0'. Now the unit tests have shown that string is Ok with it so I'm now happy to switch to string.

@liyuqian liyuqian force-pushed the base32decode branch 3 times, most recently from f539e82 to f865248 Compare September 12, 2019 21:42
For flutter/flutter#32170

This is to enable reading back SkSL persistent cache filenames
and decode them as SkData.
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Looks great. I would get rid of the explicit inlines.

@liyuqian liyuqian merged commit 8a8610a into flutter:master Sep 16, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 16, 2019
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Sep 16, 2019
git@github.com:flutter/engine.git/compare/2c4ed36c60ae...8a8610a

git log 2c4ed36..8a8610a --no-merges --oneline
2019-09-16 liyuqian@google.com Implement Base32Decode (flutter/engine#12253)
2019-09-16 liyuqian@google.com Reland "Smooth out iOS irregular input events delivery (#11817)" (flutter/engine#12280)
2019-09-16 mouad.debbar@gmail.com Refactor and polish the 'felt' tool (flutter/engine#12258)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
@liyuqian liyuqian deleted the base32decode branch September 16, 2019 23:37
Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Sep 30, 2019
git@github.com:flutter/engine.git/compare/2c4ed36c60ae...8a8610a

git log 2c4ed36..8a8610a --no-merges --oneline
2019-09-16 liyuqian@google.com Implement Base32Decode (flutter/engine#12253)
2019-09-16 liyuqian@google.com Reland "Smooth out iOS irregular input events delivery (flutter#11817)" (flutter/engine#12280)
2019-09-16 mouad.debbar@gmail.com Refactor and polish the 'felt' tool (flutter/engine#12258)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants