Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support mutiple platform/architect dependent definitions #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tw4452852
Copy link

when there are multiple platform/architect implements,
all of them will be printed.

}
)

func isPlatformDependent(f *token.File) bool {
Copy link
Owner

Choose a reason for hiding this comment

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

Unfortunately this isn't sufficient. You can have files that are platform-dependent because of build tags, not just their file names.

Copy link
Author

Choose a reason for hiding this comment

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

Yes...But now I haven't found a good way to handle this, because we use the go/build's default context which is for current GOOS and GOARCH. If we want all platform/arch associated files, we need pass all the combinations of GOOS/GOARCH/CGO... Any idea?

Copy link
Owner

Choose a reason for hiding this comment

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

If we're using the default context, how do we ever get more than one definition of a symbol with this code?

I have been pondering this issue for a while, as it also comes up in other contexts (for example, my godeps tool should really find dependencies from all architectures, which it currently does not).

Unfortunately I'm about out of time for the week now. I'll have a think over the weekend and when I get back on Monday.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I misunderstood...I have added the build tag support in the new patch set.

@tw4452852 tw4452852 force-pushed the multi-platform branch 6 times, most recently from 88f30c0 to 76939e5 Compare January 31, 2015 13:24
when there are multiple platform/architect implements,
all of them will be printed.

Signed-off-by: Tw <[email protected]>
@tw4452852
Copy link
Author

I have uploaded a new patch set, please help to review

@rogpeppe
Copy link
Owner

I'm sorry for taking so long to get around to reviewing this. I've spent
the day today looking at the issue and I'm afraid, though useful, I don't think this
change is quite what we need.

A few issues that I noted when working through this:

  1. This change works only on the final symbol and
    not on intermediate values in the expression. For example,
    consider the following:
# foo.go
package f
type foo struct {
    X int
}


# foo_windows.go
package f
type foo struct {
    X int
}

# bar.go
package f

var f foo       // def -p on "foo" prints two definitions
var g = f.X     // def -p on "X" prints only one definition

Even though there are actually two alternative definitions of foo.X
(which may well be interestingly different from one another),
the code will only print one of them because the code for traversing
the expression does not know about multiple definitions.

  1. AFAICS the redeclared method will create a circular list when called as:
    p.redeclared(ident, ident.Obj, "as package")

because redeclared assumes that its second argument is the
previous declaration, not the current one.

  1. UseAllFiles is not always the right thing to do - in some cases we
    may actually want to know the exact definition when given a certain
    set of tags.

  2. token.File.platformDep doesn't really seem like the right place to hold
    information about platform dependence - to me it seems like somewhere
    that's too low level for that information. I'm not quite sure where the
    best place would be though.

  3. I'm not even sure that "platform dependence" is the right thing to be
    triggering off. It's really "conditional-build" dependence. I see two possible
    approaches here. We could let the user define a set of possible tag
    sets and run the definition code for each set. This would be least implementation
    work, but puts the onus on the user to define appropriate tag sets, which
    is not ideal - godef is often used for exploring unfamiliar code bases.
    Another approach, which I think I favour, is to always keep around the
    complete set of definitions for an identifier - whether they're platform
    dependent or not. I started along this road trying to make ast.Scope
    hold a []_Object (I named it type Objects []_Object) but that change is
    too big for the time I have available now.
    Another possible approach which I considered was to find the set of tags
    that might influence the definition location and iterate over all possibilities of them. This
    could become computationally expensive when there are lots
    of tags in a given package, but in most cases would probably
    be trivially cheap.

I'm sorry - this isn't an easy problem, which is why I've put off doing it for so long!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants