code review 144110044: go/doc: treat _ consts as exported

go/doc: treat _ consts as exported

Fixes issue 5397.

[josharian, 2014-09-18 22:23:05]

Hello gri@golang.org (cc: golang-codereviews@googlegroups.com, rsc@golang.org),

I'd like you to review this change to
https://code.google.com/p/go

[josharian, 2014-09-18 22:23:18]

In issue 7815, Russ considered also treating struct fields named _ as exported.
This CL just does consts, but that's very easy to change.

[adg, 2014-09-18 22:32:06]

LGTM

[josharian, 2014-09-18 22:42:15]

*** Submitted as https://code.google.com/p/go/source/detail?r=f504e66669e7 ***

go/doc: treat _ consts as exported

Fixes issue 5397.

LGTM=adg
R=gri, adg
CC=golang-codereviews, rsc
https://codereview.appspot.com/144110044

[gri, 2014-09-18 22:57:01]

What happens in cases such as:

const (
   dummy T = iota
   C1
   C2
   C3
   ...
)

?

[adg, 2014-09-18 23:04:21]

On 19 September 2014 08:57, <gri@golang.org> wrote:

> What happens in cases such as:
>
> const (
>    dummy T = iota
>    C1
>    C2
>    C3
>    ...
> )
>
> ?
>

As a result of this CL? The "dummy" is still hidden. Does code like this
exist?

[gri, 2014-09-18 23:08:22]

I don't know that we have code like this, but I can easily imagine that
such code could make sense (rename dummy to some non-exported but
meaningful internal constant).

To be more precise here: In such a case, do the constants get lost in this
case? If yes, the blank fix is at best a work-around for the specific
scenario.

- gri

On Thu, Sep 18, 2014 at 4:03 PM, Andrew Gerrand <adg@golang.org> wrote:

>
> On 19 September 2014 08:57, <gri@golang.org> wrote:
>
>> What happens in cases such as:
>>
>> const (
>>    dummy T = iota
>>    C1
>>    C2
>>    C3
>>    ...
>> )
>>
>> ?
>>
>
> As a result of this CL? The "dummy" is still hidden. Does code like this
> exist?
>

[adg, 2014-09-18 23:12:00]

On 19 September 2014 09:08, Robert Griesemer <gri@golang.org> wrote:

> I don't know that we have code like this, but I can easily imagine that
> such code could make sense (rename dummy to some non-exported but
> meaningful internal constant).
>
> To be more precise here: In such a case, do the constants get lost in this
> case? If yes, the blank fix is at best a work-around for the specific
> scenario.
>

That's true. The fix for the general case would be more involved as you'd
have to look ahead at the decls in the const block to see if any are
exported.

[rsc, 2014-09-18 23:19:42]

On Thu, Sep 18, 2014 at 7:08 PM, Robert Griesemer <gri@golang.org> wrote:

> I don't know that we have code like this, but I can easily imagine that
> such code could make sense (rename dummy to some non-exported but
> meaningful internal constant).
>
> To be more precise here: In such a case, do the constants get lost in this
> case? If yes, the blank fix is at best a work-around for the specific
> scenario.
>

Fair enough, but the blank fix is a fix for code we have in the standard
library, as opposed to code we don't have anywhere (yet). It seems like a
clear win for 1.3. I'd be happy to see more general fixes instead if you
are confident in the design.

Russ

[minux, 2014-09-18 23:28:07]

On Sep 18, 2014 7:19 PM, "Russ Cox" <rsc@golang.org> wrote:
> On Thu, Sep 18, 2014 at 7:08 PM, Robert Griesemer <gri@golang.org> wrote:
>> I don't know that we have code like this, but I can easily imagine that
such code could make sense (rename dummy to some non-exported but
meaningful internal constant).
>>
>> To be more precise here: In such a case, do the constants get lost in
this case? If yes, the blank fix is at best a work-around for the specific
scenario.
>
> Fair enough, but the blank fix is a fix for code we have in the standard
library, as opposed to code we don't have anywhere (yet). It seems like a
clear win for 1.3. I'd be happy to see more general fixes instead if you
are confident in the design.
i think the essence of the problem is godoc might show a bunch of consts
without any type information (because the type might be associated with an
unexported const). This will be very confusing for the godoc reader. I
propose that godoc should always show the type if it's not shown already.
(it doesn't need to show the value if it's "hidden" by an unexported const).

IMHO that will help a lot.

[rsc, 2014-09-18 23:32:56]

On Thu, Sep 18, 2014 at 7:28 PM, minux <minux@golang.org> wrote:

>
> On Sep 18, 2014 7:19 PM, "Russ Cox" <rsc@golang.org> wrote:
> > On Thu, Sep 18, 2014 at 7:08 PM, Robert Griesemer <gri@golang.org>
> wrote:
> >> I don't know that we have code like this, but I can easily imagine that
> such code could make sense (rename dummy to some non-exported but
> meaningful internal constant).
> >>
> >> To be more precise here: In such a case, do the constants get lost in
> this case? If yes, the blank fix is at best a work-around for the specific
> scenario.
> >
> > Fair enough, but the blank fix is a fix for code we have in the standard
> library, as opposed to code we don't have anywhere (yet). It seems like a
> clear win for 1.3. I'd be happy to see more general fixes instead if you
> are confident in the design.
> i think the essence of the problem is godoc might show a bunch of consts
> without any type information (because the type might be associated with an
> unexported const). This will be very confusing for the godoc reader. I
> propose that godoc should always show the type if it's not shown already.
> (it doesn't need to show the value if it's "hidden" by an unexported const).
>
> IMHO that will help a lot.
>
there are many big fixes one could imagine. given that we're in the release
freeze i think this very minor fix is good enough and the right thing to
do, conservatively.

[gri, 2014-09-18 23:50:37]

I am not against this fix per se, it's fine for 1.4.

But there should be at least

1) a comment in the code as to why blank is special in this case; or
2) another issue expanding on the more general problem

The more general problem is that go/doc is source-code based only and has to use
(by design) a variety of heuristics. I think in this case we could do better
even with a heuristic.

https://codereview.appspot.com/143290043

[josharian, 2014-09-19 00:47:01]

> I propose that godoc should always show the type if it's not shown already.

CL 144140043 is a down payment on this; there's some very fiddly
position-adjusting to do to get the output to look right. It's in my queue of
CLs to revisit for Go 1.5.

[gri, 2014-09-19 02:49:05]

cool, great to hear! 1.5 sounds good.
- gri

On Thu, Sep 18, 2014 at 5:47 PM, <josharian@gmail.com> wrote:

> I propose that godoc should always show the type if it's not shown
>>
> already.
>
> CL 144140043 is a down payment on this; there's some very fiddly
> position-adjusting to do to get the output to look right. It's in my
> queue of CLs to revisit for Go 1.5.
>
>
> https://codereview.appspot.com/144110044/
>