Final commenting and cleanup

Author: ras0219-msft

Release/CMakeLists.txt

@@ -28,17 +28,13 @@ if(IOS)
   option(BUILD_TESTS "Build tests." ON)
 elseif(ANDROID)
   set(ANDROID_SOURCE_DIR "${CMAKE_SOURCE_DIR}/../Build_android")
-  # list(APPEND CMAKE_FIND_ROOT_PATH "${CMAKE_BINARY_DIR}/../Boost-for-Android")
-  # list(APPEND CMAKE_FIND_ROOT_PATH "${CMAKE_BINARY_DIR}/../Boost-for-Android/build")
-  # list(APPEND CMAKE_FIND_ROOT_PATH "${CMAKE_BINARY_DIR}/../Boost-for-Android/build/lib")
 
-  #set(Boost_DEBUG ON)
   set(Boost_COMPILER "-gcc")
   set(Boost_USE_STATIC_LIBS ON)
   set(BOOST_ROOT "${CMAKE_BINARY_DIR}/../Boost-for-Android/build")
   set(BOOST_LIBRARYDIR "${CMAKE_BINARY_DIR}/../Boost-for-Android/build/lib")
   find_host_package(Boost 1.55 EXACT REQUIRED COMPONENTS random system thread locale filesystem chrono atomic)
-  #find_package(Boost 1.55 EXACT REQUIRED COMPONENTS random system thread locale filesystem chrono atomic)
+
   find_package(Threads REQUIRED)
   set(OPENSSL_FOUND 1)
   set(OPENSSL_INCLUDE_DIR "${CMAKE_BINARY_DIR}/../openssl/r9d-9-armeabiv7/include")
@@ -90,12 +86,6 @@ if(ANDROID)
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -fno-strict-aliasing")
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-pedantic")
   set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-attributes -Wno-pointer-arith")
-  #set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -gcc-toolchain ${ANDROID_NDK}/toolchains/arm-linux-androideabi-4.8/prebuilt/${ANDROID_NDK_HOST_SYSTEM_NAME}")
-  #include_directories(
-  #  "${ANDROID_NDK}/sources/cxx-stl/llvm-libc++/libcxx/include"
-  #  "${ANDROID_NDK}/sources/cxx-stl/llvm-libc++/../gabi++/include"
-  #  "${ANDROID_NDK}/sources/cxx-stl/llvm-libc++/../../android/support/include"
-  #  )
   include_directories(
     "${ANDROID_NDK}/sources/cxx-stl/gnu-libstdc++/4.8/include"
     "${ANDROID_NDK}/sources/cxx-stl/gnu-libstdc++/4.8/libs/armeabi-v7a/include"

Release/include/pplx/pplxlinux.h

@@ -224,7 +224,7 @@ namespace platform
                 _M_cs.lock();
                 _M_owner = id;
                 _M_recursionCount = 1;
-            }
+            }            
         }
 
         void unlock()
@@ -238,7 +238,7 @@ namespace platform
             {
                 _M_owner = -1;
                 _M_cs.unlock();
-            }
+            }           
         }
 
     private:
@@ -312,7 +312,7 @@ namespace extensibility
 #else
     typedef details::linux_scheduler default_scheduler_t;
 #endif
-
+    
 namespace details
 {
     /// <summary>

Release/include/pplx/threadpool.h

@@ -43,6 +43,7 @@
 namespace crossplat {
 
 #if defined(ANDROID)
+// IDEA: Break this section into a separate android/jni header
 extern std::atomic<JavaVM*> JVM;
 JNIEnv* get_jvm_env();
 
@@ -130,8 +131,8 @@ class threadpool
         {
             pplx::details::platform::YieldExecution();
         }
-	// Calling get_jvm_env() here forces the thread to be attached.
-	get_jvm_env();
+        // Calling get_jvm_env() here forces the thread to be attached.
+        get_jvm_env();
         pthread_cleanup_push(detach_from_java, nullptr);
 #endif
         threadpool* _this = reinterpret_cast<threadpool*>(arg);

Release/src/CMakeLists.txt

@@ -103,7 +103,8 @@ if(WIN32)
   set_target_properties(${Casablanca_LIBRARY} PROPERTIES
     OUTPUT_NAME "${Casablanca_LIBRARY}_${CPPREST_VERSION_MAJOR}_${CPPREST_VERSION_MINOR}")
 elseif(ANDROID)
-  # Do not use SOVERSION on android
+  # Do not use SOVERSION on android. It is completely unsupported (and causes problems).
+  # Perhaps revisit in the future? (NDK r9d, 8/7/14)
 else()
   set_target_properties(${Casablanca_LIBRARY} PROPERTIES
     SOVERSION ${CPPREST_VERSION_MAJOR}.${CPPREST_VERSION_MINOR})

Release/src/json/json_parsing.cpp

@@ -34,6 +34,8 @@
 namespace crossplat
 {
 // These two are initialized in threadpool.cpp
+// HACK: This uses silently shared global variables. Revisit in the future if android
+//       JNI code gets its own header and cpp file.
 extern jmethodID java_parseDouble;
 extern jclass java_lang_double;
 }
@@ -406,15 +408,27 @@ namespace
 #ifdef ANDROID
     static double anystod(const char* str)
     {
+        // HACK: To work around issues with bionic's strtod() implementation, we ship
+        //       the string to parse into java, call the java function
+        //       java.lang.Double.parseDouble(), and bring it back down to native code.
+        //
+        // The strtod() bug causes problems when parsing large strings; furthermore, it
+        // is arguable that it uses "locale" information, which is announced as being
+        // off-limits for native code.
+        //
+        // It is highly unlikely this situation will be remedied due to ecosystem
+        // fragmentation.
         if (str == nullptr)
             str = "";
 
-	auto env = crossplat::get_jvm_env();
+        auto env = crossplat::get_jvm_env();
 
         jdouble parsed_double;
 
         crossplat::java_local_ref<jstring> jstr{env->NewStringUTF(str)};
-        parsed_double = env->CallStaticDoubleMethod(crossplat::java_lang_double, crossplat::java_parseDouble, jstr.get());
+        parsed_double = env->CallStaticDoubleMethod(crossplat::java_lang_double,
+                                                    crossplat::java_parseDouble,
+                                                    jstr.get());
 
         jthrowable exc = env->ExceptionOccurred();
         if (exc)

Release/src/json/json_serialization.cpp

@@ -34,6 +34,7 @@ using namespace web::json;
 using namespace utility;
 using namespace utility::conversions;
 
+
 //
 // JSON Serialization
 //

Release/src/utilities/asyncrt_utils.cpp

@@ -883,6 +883,15 @@ datetime __cdecl datetime::from_string(const utility::string_t& dateString, date
     }
 
 #if defined(ANDROID)
+    // HACK: The (nonportable?) POSIX function timegm is not available in
+    //       bionic. As a workaround[1][2], we set the C library timezone to
+    //       UTC, call mktime, then set the timezone back. However, the C
+    //       environment is fundamentally a shared global resource and thread-
+    //       unsafe. We can protect our usage here, however any other code might
+    //       manipulate the environment at the same time.
+    //
+    // [1] http://linux.die.net/man/3/timegm
+    // [2] http://www.gnu.org/software/libc/manual/html_node/Broken_002ddown-Time.html
     time_t time;
 
     static boost::mutex env_var_lock;

Release/tests/Common/UnitTestpp/src/ExecuteTest.h

@@ -69,7 +69,7 @@ void ExecuteTest(T& testObject, TestDetails const& details, bool isMockTest)
 #else
 		UT_TRY
 		({
-                        UNITTEST_THROW_SIGNALS_POSIX_ONLY
+			UNITTEST_THROW_SIGNALS_POSIX_ONLY
 			testObject.RunImpl();
 		})
 #endif

Release/tests/Functional/http/client/http_client_tests.h

@@ -46,4 +46,4 @@ void test_connection(tests::functional::http::utilities::test_http_server *p_ser
 // Take in the path to request and what path should be received in the server.
 void test_connection(tests::functional::http::utilities::test_http_server *p_server, web::http::client::http_client *p_client, const utility::string_t &request_path, const utility::string_t &expected_path);
 
-}}}}
+}}}}

Release/tests/Functional/http/client/multiple_requests.cpp

@@ -154,7 +154,8 @@ TEST_FIXTURE(uri_address, requests_with_data)
         try
         {
             http_asserts::assert_response_equals(responses[i].get(), code);
-        } catch (...)
+        }
+        catch (...)
         {
             VERIFY_ARE_EQUAL(1, 0);
         }
@@ -198,7 +199,8 @@ TEST_FIXTURE(uri_address, responses_with_data)
             http_response rsp = responses[i].get();
             http_asserts::assert_response_equals(rsp, code, headers);
             VERIFY_ARE_EQUAL(to_string_t(request_body), rsp.extract_string().get());
-        } catch (...)
+        }
+        catch (...)
         {
             VERIFY_ARE_EQUAL(1, 0);
         }