Rapidjson parity by ccaraman · Pull Request #205 · envoyproxy/envoy

ccaraman · 2016-11-09T22:34:41Z

No description provided.

mattklein123 · 2016-11-09T23:04:34Z

source/common/json/json_loader.cc

-                                error.line, error.column));
+  char buffer[4096];
+
+  FILE* fp = fopen(file_path.c_str(), "r");


You are leaking here. Can you use C++ iostream?

if not, you can use CSmartPtr and supply fclose not to leak fd.

Was able to switch to iostream.

mattklein123 · 2016-11-09T23:06:00Z

source/common/json/json_loader.cc

+    throw Exception(fmt::format("File:{} doesn't exist.", file_path));
+  }
+
+  rapidjson::FileReadStream fs(fp, buffer, sizeof(buffer));


what is buffer used for here? Why does it have to be specified? Why 4K? Seems odd there is no API to just deal with buffering internally.

Removed. Switched to using iostream.

mattklein123 · 2016-11-09T23:06:20Z

source/common/json/json_loader.cc

+  rapidjson::FileReadStream fs(fp, buffer, sizeof(buffer));
+  if (document_.ParseStream(fs).HasParseError()) {
+    throw Exception(fmt::format("Error(offset {}): {}\n",
+                                static_cast<unsigned>(document_.GetErrorOffset()),


What is offset? Does it include line number?

wondering, why casting to unsigned_int?

mattklein123 · 2016-11-09T23:07:21Z

source/exe/CMakeLists.txt

 target_link_libraries(envoy dl)

 include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR})
+include_directories(SYSTEM ${ENVOY_RAPIDJSON_INCLUDE_DIR})


why is this needed?

Removed. It was due to the initial integration of rapidjson. No longer needed now that rapidjson headers are only in the .cc file.

mattklein123 · 2016-11-09T23:07:29Z

source/server/CMakeLists.txt

 include_directories(${ENVOY_NGHTTP2_INCLUDE_DIR})
 include_directories(SYSTEM ${ENVOY_OPENSSL_INCLUDE_DIR})
 include_directories(SYSTEM ${ENVOY_LIGHTSTEP_TRACER_INCLUDE_DIR})
+include_directories(SYSTEM ${ENVOY_RAPIDJSON_INCLUDE_DIR})


why is this needed?

It made the compiler happy, back when I was playing with it. I don't think this will be necessary once I find a way to get rid of the refernce to rapidjson/document.h in the json_loader.h

mattklein123 · 2016-11-09T23:07:50Z

source/common/json/json_loader.h


 protected:
-  Object() : name_("root") {}
+  Object() : name_("root"), document_() {}


default initializer not needed

mattklein123 · 2016-11-09T23:08:08Z

source/common/json/json_loader.h

+  }
+
+  // Empty Object
+  Object(const std::string& name) : name_(name), document_() {}


default initializer not needed

mattklein123 · 2016-11-09T23:08:24Z

source/common/json/json_loader.h

 class Object {
 public:
-  Object(json_t* json, const std::string& name) : json_(json), name_(name) {}
+  Object(const rapidjson::Value& value, const std::string& name) : name_(name), document_() {


default initializer not needed

mattklein123 · 2016-11-09T23:09:05Z

source/common/json/json_loader.h

-struct json_t;
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wold-style-cast"
+#include "rapidjson/document.h"


Do not leak this in header. Find a way around this even if you need to expose an interface.

mattklein123 · 2016-11-09T23:11:43Z

source/common/json/json_loader.cc


 bool Object::hasObject(const std::string& name) const {
-  return json_object_get(json_, name.c_str()) != nullptr;
+  return document_.HasMember(name.c_str()) && document_.IsObject();


Don't know API but seems wrong to be calling document_.IsObject() here.

mattklein123 · 2016-11-09T23:13:12Z

source/common/json/json_loader.cc

 double Object::getDouble(const std::string& name) const {
-  json_t* real = json_object_get(json_, name.c_str());
-  if (!real || !json_is_real(real)) {
+  if (!document_.HasMember(name.c_str()) || !document_[name.c_str()].IsDouble()) {


can you avoid multiple hash lookups here and elsewhere

Removed multiple hash lookups.

RomanDzhabarov · 2016-11-10T00:08:43Z

source/common/json/json_loader.cc

 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wold-style-cast"
-#include "jansson.h"
+#include <rapidjson/error/en.h>


minor: s/</"

RomanDzhabarov · 2016-11-21T23:52:05Z

include/envoy/json/json_object.h

nit: two spaces :-)

RomanDzhabarov · 2016-11-21T23:55:19Z

source/common/json/json_loader.cc

nit, i'd reserve vector capacity upfront as we know the Size.

Fixed here and in json_loader.h

RomanDzhabarov · 2016-11-22T00:10:58Z

source/common/json/json_loader.cc

not to familiar with api, but can we

auto &blah = document_[name.c_str()];
and query internal map or something once.

if (!blah.IsBool()) {
}

return blah.GetBool();

mattklein123 · 2016-11-22T02:06:03Z

docs/install/requirements.rst

delete all references to jansson from docs, cmake, etc.

I will do that clean up in Jira 615.

mattklein123 · 2016-11-22T02:06:29Z

include/envoy/json/json_object.h

mattklein123 · 2016-11-22T02:06:43Z

include/envoy/json/json_object.h

mattklein123 · 2016-11-22T02:06:50Z

include/envoy/json/json_object.h

mattklein123 · 2016-11-22T02:07:25Z

include/envoy/json/json_object.h

wrong type / please fix grammar

mattklein123 · 2016-11-22T02:09:45Z

include/envoy/json/json_object.h

can't this be implemented in terms of iterate? Why is this needed?

Or get rid of iterate() from the public interface

mattklein123 · 2016-11-22T02:12:32Z

source/common/json/json_loader.cc

We should not be copying anything. If needed a reference to the parent document should be used.

mattklein123 · 2016-11-22T02:13:08Z

source/common/json/json_loader.cc

mattklein123 · 2016-11-22T02:14:26Z

test/common/json/json_loader_test.cc

what was this test testing and why is it no longer failing

This was testing Max and Min Integer. Unfortunately, rapidjson doesn't throw an exception during Parsing. We should push this validation to the schema. :(

mattklein123 · 2016-11-22T02:14:45Z

source/common/json/json_loader.h

initializer list

mattklein123 · 2016-11-22T02:14:50Z

source/common/json/json_loader.h

initializer list

mattklein123 · 2016-11-22T02:15:56Z

source/common/json/json_loader.h

reserve capacity ahead of time

mattklein123 · 2016-11-22T02:16:10Z

source/common/json/json_loader.h

local var not needed

removed extra local var.

mattklein123 · 2016-11-22T02:16:20Z

source/common/json/json_loader.h

mattklein123 · 2016-11-22T02:19:26Z

source/common/json/json_loader.h

initializer list

mattklein123 · 2016-11-22T02:19:38Z

source/common/json/json_loader.cc

mattklein123 · 2016-11-30T01:15:09Z

source/common/json/json_loader.h

-struct json_t;
-
 namespace Json {
+class Object;


split object out into public include and only define the factory stuff in this file.

mattklein123 · 2016-11-30T01:20:38Z

test/common/json/json_loader_test.cc

-  }
-
-  {
-    EXPECT_THROW(StringLoader("{\"val\":9223372036854775808}"), Exception);


What actually happens here? We shouldn't delete this test.

…#205)

* worker: provide removeFilterChain interface (envoyproxy#10528) Signed-off-by: Yuchen Dai <silentdai@gmail.com> * cherry-pick in place filter chain update Signed-off-by: Yuchen Dai <silentdai@gmail.com> * fix conficts Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Signed-off-by: Jose Nino jnino@lyft.com Description: previously the CI runs would only build the demos. However we did not run them, or check for liveliness. This PR adds running all the Android demos in the emulator for Mac CI. Risk Level: low - improved CI Testing: tested locally and on CI Fixes #117 Signed-off-by: JP Simard <jp@jpsim.com>

Co-authored-by: Matteo Pace <pace.matteo96@gmail.com>

**Commit Message**: This refactors translator by removing the unnecessary reflect as well as the slice variable copies. Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

mattklein123 reviewed Nov 9, 2016

View reviewed changes

RomanDzhabarov reviewed Nov 10, 2016

View reviewed changes

ccaraman force-pushed the rapidjson-parity branch from b92feb0 to bac2199 Compare November 21, 2016 22:09

RomanDzhabarov reviewed Nov 22, 2016

View reviewed changes

mattklein123 reviewed Nov 22, 2016

View reviewed changes

include/envoy/json/json_object.h Outdated

Copy link
Copy Markdown

Member

mattklein123 Nov 22, 2016

Choose a reason for hiding this comment

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

wrong type

mattklein123 reviewed Nov 22, 2016

View reviewed changes

include/envoy/json/json_object.h Outdated

Copy link
Copy Markdown

Member

mattklein123 Nov 22, 2016

Choose a reason for hiding this comment

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

wrong type / please fix grammar

mattklein123 reviewed Nov 22, 2016

View reviewed changes

source/common/json/json_loader.cc Outdated

Copy link
Copy Markdown

Member

mattklein123 Nov 22, 2016

Choose a reason for hiding this comment

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

no copying

mattklein123 reviewed Nov 22, 2016

View reviewed changes

source/common/json/json_loader.cc Outdated

Copy link
Copy Markdown

Member

mattklein123 Nov 22, 2016

Choose a reason for hiding this comment

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

const

ccaraman added 15 commits November 29, 2016 16:35

two

a6c4233

building!

a96e8b8

cleanish

0934352

fix headers

0304a73

clean

d5f629a

destructor

7759382

fmt format

dd52b40

documentation

9d21f6a

reference

4e43247

shared ptr

25ed966

address prr

84f0976

comments

9065001

push

ba21f10

finally

c93eca2

merge

424f6db

ccaraman force-pushed the rapidjson-parity branch from f311925 to 424f6db Compare November 30, 2016 01:04

mattklein123 reviewed Nov 30, 2016

View reviewed changes

one more

64bf2db

mattklein123 approved these changes Nov 30, 2016

View reviewed changes

mattklein123 merged commit 88ed42a into master Nov 30, 2016

mattklein123 deleted the rapidjson-parity branch November 30, 2016 03:04

rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018

add mixer_client_config_proto build target for istio/api. (envoyproxy…

b2d9149

…#205)

arminabf pushed a commit to arminabf/envoy that referenced this pull request Jun 5, 2024

Fix usage examples in README. (envoyproxy#205)

f5f890a

Co-authored-by: Matteo Pace <pace.matteo96@gmail.com>

Conversation

ccaraman commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment