I suggest moving this to compare.go, and it seems worth including some tests for it on its own. Code-wise, it is very similar to clone and extend in the blueprint/proptools package, so maybe move it there, and try to make them structured similarly?

good idea

Go doesn't have objects or classes.
s/class/type/
s/object/value/

ok

This conversion is unnecessary, use
reflect.DeepEqual(actual.item.Interface(), expected.item.Interface())

cool

Well I investigated a little further but it seems that unfortunately, x.Interface() fails if there were any private fields traversed while accessing x. My thought now is that it's preferable to have support for diffs of private fields if it requires reimplementing this method like this.

This is unnecessary, use:
if !b.MapIndex(aKey).IsValid()

See below about keys being pointers

What happens if two deeply equal pointers are keys in the same map? Only the pointer value will be hashed, so they will still be two different keys.

Yeah exactly; the test doesn't confirm that the pointers themselves are correct, only that they have a corresponding value in the other map.

Do you mean that we want to fix deepCompare for the following case?

////
var a and b, deeply equal to each other
var c and d, deeply equal to each other

map1{a:true, b:true, c:true}
map2{a:true, c:true, d:true}

?

for which deepCompare would return true when really we want false

I guess it could also be worth fixing this case too:

////
var a and b, deeply equal to each other
map1{a:true, b:false}
map2{a:true, b:false}

for which we want an answer of true but theoretically get a random answer (though I expect in practice it's always false).

Is that what you're referring to?

We would need to define what the semantics are for comparing two maps whose keys are deepequal but not equal. Even for two maps that were deep-cloned off of each other, there is no guarantee that we can determine a 1-1 mapping between keys in the two maps.

The alternative is to sidestep this problem and refuse to deepcompare pointers or anything that might contain pointers (structs, arrays) as the keys in maps. This should be fine for the parser/printer, since they only support strings as the keys to maps.

Unfortunately the test needs to be able to compare SyntaxTree.comments, whose type is map[ParseNode](*CommentPair) , which uses an interface type as its key

I think that my inclination, if we want to spend the effort, would be two changes. 1. we fix the case in deepCompare involving maps having recursively equal keys such that deepCompare deems them equal, and 2. we require the individual tests to explicitly invoke deepCompare, so if the tests require certain relationships between the pointers themselves (currently the checks specific to the pointers are in the Builder class itself), then the tests can validate those requirements separately, and can still use deepCompare while doing it.

The changes to deepCompare I imagine would be to have it assume that if there exists a 1-1 mapping for which we cannot find a relevant difference, then we deem the data structures equivalent. In other words: 1. For each combination of entries in map1 and map2, check whether those two entries are equal. 2. For each entry A in map1 that has K matches in map2, confirm that there are exactly K such entries in map1 that have the same set of matches in map2 3. If every condition in 2 yielded true, then return true.

As for sidestepping the problem, my favorite option is to invoke the printer (or perhaps VerbosePrinter) in printer_test.go and check the text output.

Alternatively we could adjust the code being tested attach an integer id to each ParseNode and to use that as the map key rather than the ParseNode reference itself.

I think this function (and findMatching and compareValues) are unnecessary due to comments below

See below about keys being pointers

Don't cast to float, either implement a trivial min function or open code it

ok

Or:
for i := 0; i < a.Len() && i < b.Len(); i++ {

I'll re-implement the 'min' function as you mentioned above.

I extracted the 'min' operation to slightly decrease the number of values being recomputed during each loop iteration (I don't see anything in https://github.com/golang/go/wiki/CompilerOptimizations specifying that the Go compiler can optimize 'i < a.Len() && i < b.Len()').

It would be more reliable to compare the types here, or delete the check entirely

ok

Deep comparing map keys seems unnecessary to me. If they hash to the same value they must be == equal, and if they are == equal then they must be DeepEqual. If that is not true, can you construct a test that returns different values for reflect.DeepEqual and deepCompare?

Yeah, I was surprised too. I'll add a test involving a map having keys of pointer type.

Why check in a deprecated function?

I'll remove it

reordering these makes this really hard to review

done

The distinction between ParseTree and SyntaxTree is not clear to me. Leaving this as File and File.Name would be much easier to review, and then you can use a separate patch to do renames. The gorename command line tool is useful for doing programmatic renames, and you can include the gorename command in the commit message.

done

Imports for blueprint modules should go after all the imports from the standard library, with a blank line between the groups.

Have you made any customizations to Gogland that automate things like line lengths and import ordering? If so, I wonder if we can check them in somewhere.

No, and it keeps screwing them up for me too.

Might be worth at least adding sanity checks here. If len(expectedNodes) < len(actualNodes) this will panic

ok

Go doesn't enforce a hard line length limit, but try to keep lines under 100 characters.

Also Go doesn't use underscores in variable names.

Yeah I don't like the underscore either; I just use it to clarify that this is the remappings of the actualNodes as opposed to being the actual nodeRemappings. What name would you recommend that conveys as much information?

Sometimes its better to use a short, readable name and put the extra information in a comment, rather than carrying all the information everywhere you use the name. See https://github.com/golang/go/wiki/CodeReviewComments#variable-names

Hmm it seems to me that the trouble in this case is more that I forgot to add a comment than that I used a long variable name.

In any case, I've now redone it in the next version of this commit for you to take a look at.

NamedItem isn't making the API any clearer, maybe just pass the string and tree as adjacent parameters

ok

Should this be actual.SourcePositions?

true

This doesn't seem worth enforcing, just strip it if it is there.

ok

a two-value return is more idiomatic than returning a struct

When is that (two values preferable to a struct) the case? Is it when the two values have fundamentally different meanings? It seems to me that a Comparison is itself a concept. If we wanted to instead return an entire list of differences, it'd be convenient to not have to update the signatures of each method based on the implementation of a Comparison.

I'd say that a struct should be used if you are going to pass that encapsulated value around (more than just returning it). If you are going to immediately unpack and operate on it, a multiple-value return is better.

That logic sounds like a decent rule of thumb to me: to use a multi-valued return if the caller is likely to immediately read all of the contained fields. Does it apply here? None of the functions in compare.go read the field Comparison.Difference .

If for example we wanted to add another field to Comparison that perhaps specified the property path of the difference, then those fields probably wouldn't be read by anything in compare.go either.

I talked to Dan and he thinks that if the number of fields is small then the thing to do is to make the fields be separate return values, and if the number of fields grows then to consider making the return be a struct at that time. I suppose that that's enough of a rule of thumb for me to be able to follow anyway, so I'll do that.

document the semantics of this comparison, especially as compared to reflect.DeepEqual

done

compares two arrays or slices

document that this compares [0,len()), and not [0,cap())

ok

compares two pointers or interfaces

done

This seems both unnecessary and misleading, since same() and different() are not public

oops

return false, sort.Strings(differences)[0]

thanks

arrays or slices

ok

This seems like a bug?

What would you prefer instead?

These results contain %!t, which is an error inserted into the result of fmt.Sprintf. %t expects a bool, did you mean %T?

Nil check is unnecessary, range on a nil slice is not an error

Hmm, when I remove the check, I get a panic trying to access comments.preComments

These results contain %!t, which is an error inserted into the result of fmt.Sprintf. %t expects a bool, did you mean %T?