FAQ
https://codereview.appspot.com/13270045/diff/15001/oracle/json.go
File oracle/json.go (right):

https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode19
oracle/json.go:19: type PeersJSON struct {
Do these types need to be exported? Is is enough for the encoding/json
package to export the fields?

https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode32
oracle/json.go:32: // A 'callees' query returns a CalleesJSON.
These comments should start with the type. Something like

// A CalleesJSON is returned by a 'callees' query.

https://codereview.appspot.com/13270045/diff/15001/oracle/oracle.go
File oracle/oracle.go (right):

https://codereview.appspot.com/13270045/diff/15001/oracle/oracle.go#newcode210
oracle/oracle.go:210: func (res *Result) ToResultJSON() *ResultJSON {
I'm not sure I understand the purpose of this. Why not just func (res
*Result) MarshalJSON ([]byte, error)

https://codereview.appspot.com/13270045/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Adonovan at Sep 3, 2013 at 2:20 pm
    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go
    File oracle/json.go (right):

    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode19
    oracle/json.go:19: type PeersJSON struct {
    On 2013/09/03 13:16:55, crawshaw1 wrote:
    Do these types need to be exported? Is is enough for the encoding/json package
    to export the fields?
    It's true that encoding/json need only see the fields, but these
    structures still ought to be public, for two reasons, the most obvious
    being that this is the definition of a public interface to the package!
    Whether or not you actually write Go code to link against these types,
    these structures are very much exported. Secondarily, one might
    actually want to write a Go client of this interface (such as the
    tests).

    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode32
    oracle/json.go:32: // A 'callees' query returns a CalleesJSON.
    On 2013/09/03 13:16:55, crawshaw1 wrote:
    These comments should start with the type. Something like
    // A CalleesJSON is returned by a 'callees' query.
    Done, throughout.

    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode197
    oracle/json.go:197: DescribePackage *DescribePackageJSON
    `json:"describepkg,omitempty"`
    I've grouped together all the 'describe' results in a DescribeJSON so
    now it's a two-level tagged union. This is cleaner.

    https://codereview.appspot.com/13270045/diff/15001/oracle/oracle.go
    File oracle/oracle.go (right):

    https://codereview.appspot.com/13270045/diff/15001/oracle/oracle.go#newcode210
    oracle/oracle.go:210: func (res *Result) ToResultJSON() *ResultJSON {
    On 2013/09/03 13:16:55, crawshaw1 wrote:
    I'm not sure I understand the purpose of this. Why not just func (res *Result)
    MarshalJSON ([]byte, error)
    To kill two birds with one stone: not only do the JSON structs help to
    implement the JSON wire format but they also define the public API to
    this package.

    Would it make more sense to expose the following methods of Result:

        func (*Result) ToResultJSON() *ResultJSON

        // Delegates to ToResultJSON
        func (*Result) MarshalJSON() ([]byte, error)

    and eliminate this utility function:
        func (*ResultJSON) ToIndentedBytes() ([]byte, error)
    ?

    https://codereview.appspot.com/13270045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • David Crawshaw at Sep 3, 2013 at 2:36 pm
    If ResultJSON is the exported interface to be used as an API by other
    Go programs, then how about merging it with Result, and removing the
    JSON suffix from the other types?
    On Tue, Sep 3, 2013 at 10:20 AM, wrote:

    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go
    File oracle/json.go (right):

    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode19
    oracle/json.go:19: type PeersJSON struct {
    On 2013/09/03 13:16:55, crawshaw1 wrote:

    Do these types need to be exported? Is is enough for the encoding/json package
    to export the fields?

    It's true that encoding/json need only see the fields, but these
    structures still ought to be public, for two reasons, the most obvious
    being that this is the definition of a public interface to the package!
    Whether or not you actually write Go code to link against these types,
    these structures are very much exported. Secondarily, one might
    actually want to write a Go client of this interface (such as the
    tests).


    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode32
    oracle/json.go:32: // A 'callees' query returns a CalleesJSON.
    On 2013/09/03 13:16:55, crawshaw1 wrote:

    These comments should start with the type. Something like
    // A CalleesJSON is returned by a 'callees' query.

    Done, throughout.

    https://codereview.appspot.com/13270045/diff/15001/oracle/json.go#newcode197
    oracle/json.go:197: DescribePackage *DescribePackageJSON
    `json:"describepkg,omitempty"`
    I've grouped together all the 'describe' results in a DescribeJSON so
    now it's a two-level tagged union. This is cleaner.


    https://codereview.appspot.com/13270045/diff/15001/oracle/oracle.go
    File oracle/oracle.go (right):

    https://codereview.appspot.com/13270045/diff/15001/oracle/oracle.go#newcode210
    oracle/oracle.go:210: func (res *Result) ToResultJSON() *ResultJSON {
    On 2013/09/03 13:16:55, crawshaw1 wrote:

    I'm not sure I understand the purpose of this. Why not just func (res *Result)
    MarshalJSON ([]byte, error)

    To kill two birds with one stone: not only do the JSON structs help to
    implement the JSON wire format but they also define the public API to
    this package.

    Would it make more sense to expose the following methods of Result:

    func (*Result) ToResultJSON() *ResultJSON

    // Delegates to ToResultJSON
    func (*Result) MarshalJSON() ([]byte, error)

    and eliminate this utility function:
    func (*ResultJSON) ToIndentedBytes() ([]byte, error)
    ?

    https://codereview.appspot.com/13270045/
    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Adonovan at Sep 3, 2013 at 3:59 pm

    On 2013/09/03 14:36:07, crawshaw1 wrote:
    If ResultJSON is the exported interface to be used as an API by other
    Go programs, then how about merging it with Result, and removing the
    JSON suffix from the other types?
    Per your and sameer's offline comments, I've moved the JSON types into a
    subpackage, and made the Result structure implement the
    encoding/json.Marshaler interface so that you don't need to import the
    subpackage if you just want to serialize a Result to bytes, but they're
    available if you want to deserialize bytes.

    PTAL


    https://codereview.appspot.com/13270045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Crawshaw at Sep 3, 2013 at 7:20 pm
    LGTM

    https://codereview.appspot.com/13270045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Adonovan at Sep 3, 2013 at 7:29 pm
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=e3058b01f286&repo=tools ***

    go.tools/oracle: add option to output results in JSON syntax.

    See json.go for interface specification.

    Example usage:
    % oracle -format=json -mode=callgraph
    code.google.com/p/go.tools/cmd/oracle

    + Tests, based on (small) golden files.

    Overview:
        Each <query>Result structure has been "lowered" so that all
        but the most trivial logic in each display() function has
        been moved to the main query.

        Each one now has a toJSON method that populates a json.Result
        struct. Though the <query>Result structs are similar to the
        correponding JSON protocol, they're not close enough to be
        used directly; for example, the former contain richer
        semantic entities (token.Pos, ast.Expr, ssa.Value,
        pointer.Pointer, etc) whereas JSON contains only their
        printed forms using Go basic types.

        The choices of what levels of abstractions the two sets of
        structs should have is somewhat arbitrary. We may want
        richer information in the JSON output in future.

    Details:
    - oracle.Main has been split into oracle.Query() and the
        printing of the oracle.Result.
    - the display() method no longer needs an *oracle param, only
        a print function.
    - callees: sort the result for determinism.
    - callees: compute the union across all contexts.
    - callers: sort the results for determinism.
    - describe(package): fixed a bug in the predicate for method
        accessibility: an unexported method defined in pkg A may
        belong to a type defined in package B (via
        embedding/promotion) and may thus be accessible to A. New
        accessibleMethods() utility fixes this.
    - describe(type): filter methods by accessibility.
    - added tests of 'callgraph'.
    - pointer: eliminated the 'caller CallGraphNode' parameter from
        pointer.Context.Call callback since it was redundant w.r.t
        site.Caller().
    - added warning if CGO_ENABLED is unset.

    R=crawshaw
    CC=golang-dev
    https://codereview.appspot.com/13270045


    https://codereview.appspot.com/13270045/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 3, '13 at 1:17p
activeSep 3, '13 at 7:29p
posts6
users2
websitegolang.org

2 users in discussion

Crawshaw: 3 posts Adonovan: 3 posts

People

Translate

site design / logo © 2022 Grokbase