Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: `TDocVariantData.B` or `GetBooleanByName` couldn't correctly handle the "true" and "false" JSON values. #229

Open
wants to merge 5 commits into
base: master
from

Conversation

@edwinyzh
Copy link
Contributor

commented Aug 3, 2019

No description provided.

edwinyzh added some commits Aug 3, 2019

Merge pull request #3 from synopse/master
Pull the latest revision from upstream synopse/mORMot
@@ -21321,6 +21321,8 @@ function VariantToBoolean(const V: Variant; var Value: Boolean): boolean;
Value := TVarData(V).VBoolean;
varInteger: // coming e.g. from GetJsonField()
Value := TVarData(V).VInteger=1;
varString, varUString: // handles "true"/"false" values in JSON

This comment has been minimized.

Copy link
@synopse

synopse Aug 3, 2019

Owner

varUString should be used only with a proper {$ifdef HASVARUSTRING} {$endif} confitional.

@@ -21321,6 +21321,8 @@ function VariantToBoolean(const V: Variant; var Value: Boolean): boolean;
Value := TVarData(V).VBoolean;
varInteger: // coming e.g. from GetJsonField()
Value := TVarData(V).VInteger=1;
varString, varUString: // handles "true"/"false" values in JSON
Value := SameTextU(VariantToUTF8(V), BOOL_UTF8[True]);

This comment has been minimized.

Copy link
@synopse

synopse Aug 3, 2019

Owner

this will create a temporary string on the stack, which slows down this function a lot (hidden try...finally block)

synopse pushed a commit that referenced this pull request Aug 3, 2019

@synopse

This comment has been minimized.

Copy link
Owner

commented Aug 3, 2019

I guess 99e8896 properly implement what you expected

Improvements requested by @ab to the previous commit for the `Variant…
…ToBoolean` function: Optimization and take into account the `HASVARUSTRING` conditional define
@edwinyzh

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

I guess 99e8896 properly implement what you expected

Thanks ab! I was amending the code as per your request above, and it's good you do it by yourself :)
Tested and it works.

edwinyzh added some commits Aug 3, 2019

@ab has done the fix on his side, so overwrite my previous changes to…
… my own fork, so that there will be no conflicts when merging the upstream repo into my forked repo.
Merge pull request #4 from synopse/master
pull the latest from upstream repo - let VariantToBoolean() handle text input as potential boolean value
@ab

This comment has been minimized.

Copy link

commented on cd48761 Aug 22, 2019

You must mean some other @ab.

This comment has been minimized.

Copy link
Owner Author

replied Aug 22, 2019

Hah, yes, sorry :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.