Skip to content

Conversation

Vhuynh25
Copy link
Contributor

@Vhuynh25 Vhuynh25 commented Feb 1, 2022

No description provided.

@crow-clang-format
Copy link

--- include/crow/json.h	(before formatting)
+++ include/crow/json.h	(after formatting)
@@ -1774,8 +1774,9 @@
                     {
                         if (v.nt == num_type::Floating_point)
                         {
-                            if(isnan(v.num.d) || isinf(v.num.d)){
-                                CROW_LOG_WARNING << "Invalid JSON value detected (" << v.num.d << "), ignoring" ;
+                            if (isnan(v.num.d) || isinf(v.num.d))
+                            {
+                                CROW_LOG_WARNING << "Invalid JSON value detected (" << v.num.d << "), ignoring";
                                 break;
                             }
 #ifdef _MSC_VER

@epajarre
Copy link
Contributor

epajarre commented Feb 2, 2022 via email

@The-EDev
Copy link
Member

The-EDev commented Feb 2, 2022

@epajarre #315 did output null instead of omitting the value outright, unfortunately either returning null or omitting the value will cause headaches to an unsuspecting developer, hence the warning.

Whether or not this PR would cause broken syntax or not I've yet to test. But I will look into it.

@crow-clang-format
Copy link

--- include/crow/json.h	(before formatting)
+++ include/crow/json.h	(after formatting)
@@ -1774,7 +1774,7 @@
                     {
                         if (v.nt == num_type::Floating_point)
                         {
-                            if(isnan(v.num.d) || isinf(v.num.d))
+                            if (isnan(v.num.d) || isinf(v.num.d))
                             {
                                 CROW_LOG_WARNING << "Invalid JSON value detected (" << v.num.d << "), ignoring";
                                 break;

@The-EDev
Copy link
Member

The-EDev commented Feb 7, 2022

@epajarre I've recently had time to test the PR and you were right, it does seem to produce incorrect JSON whether the invalid value is in an object or an array.

It seems that omitting the value (and key) properly will take more work than just outputting null. That and the Array use case make me rethink my stance on #315. It might actually be best to replace an invalid number with null while still sending out a warning which explains that the invalid value was replaced with null.

@Vhuynh25 I am very sorry for this change in scope and taking your time, Would you be interested in changing your code to output null instead of simply doing nothing when an invalid value is detected (and change the warning message accordingly).

Additionally for some reason I couldn't compile Crow locally until I changed cmath to math.h. I'm not sure why that was a problem but I'd appreciate it if you could look into that.

@The-EDev
Copy link
Member

The-EDev commented Feb 9, 2022

Thank you for your help @Vhuynh25, and for adapting your code to the change in scope.

I'll run my local tests to check that everything is in order (and possibly push those tests as unit tests) and merge the PR.

@The-EDev The-EDev merged commit e8e4626 into CrowCpp:master Feb 9, 2022
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.

json::wvalue incorrectly outputs NaN or infinite values instead of ignoring them
3 participants