Sam Okrent has posted comments on this change. Change subject: Add json builder utility ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7387/1/src/kudu/util/jsonbuilder.h File src/kudu/util/jsonbuilder.h: Line 1: // Licensed to the Apache Software Foundation (ASF) under one > Should this file be named easy_json.[h,cc]? I thought maybe it should be, but I was trying to stay consistent with jsonreader and jsonwriter, and highlight that it's supposed to be used for building. Should I change it? Line 17: #ifndef KUDU_UTIL_JSONBUILDER_H_ > Use '#pragma once' for new code. Done Line 29: // Intended solely for building json objects, not writing/parsing. > If that's the case, then maybe it's best to leave the getters out? It seem Done Line 71: EasyJson& SetObject(); > In terms of functionality, are these setters providing more than the index SetObject() and SetArray()? No, it's not necessary to call them, but I thought it might improve client code clarity to call them explicitly sometimes. Should I get rid of them? Line 102: scoped_refptr<EasyJsonAllocator> alloc_; > Prefer std::shared_ptr to scoped_refptr for new code. Ok, I was following the style guide (http://kudu.apache.org/docs/contributing.html#_code_style), which I believe says prefer scoped_refptr. But shared_ptr instead? -- To view, visit http://gerrit.cloudera.org:8080/7387 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie195b06c5ba67af3043d2032c56b90c1a85f641a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sam Okrent <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sam Okrent <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
