This is an archive of the discontinued LLVM Phabricator instance.

[ADT][NFC] Correct the wrong header comment of ImmutableSet::add_internal
Needs ReviewPublic

Authored by martong on Jul 20 2021, 8:19 AM.

Details

Summary

ImmutableSet and ImmutableMap add function boils down to add_internal. The
related comments for that function are however misleading. We do return a new
tree if an element with an existing key (and possibly with a different value)
is added. We do not return the old tree with the old key-value pair.

Diff Detail

Event Timeline

martong created this revision.Jul 20 2021, 8:19 AM
martong requested review of this revision.Jul 20 2021, 8:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2021, 8:19 AM
martong retitled this revision from [ADT] Correct the wrong header comment of ImmutableSet::add_internal to [ADT][NFC] Correct the wrong header comment of ImmutableSet::add_internal.Jul 20 2021, 8:32 AM
vsavchenko added inline comments.Jul 20 2021, 8:52 AM
llvm/include/llvm/ADT/ImmutableSet.h
541–542

It looks that it worked that way before, createNode (or Create, as it was called before) was checking in the cache if we have a tree like this. Now it's all done higher up the hierarchy of calls in getCanonicalTree.
Maybe we should just remove this sentence? Because why would we mention the case that is actually never covered (and it is not covered because it is talking about the data item not the key).

martong added inline comments.Jul 21 2021, 7:00 AM
llvm/include/llvm/ADT/ImmutableSet.h
541–542

Perhaps, the data item in the comment wrongly refers the value_type_ref V parameter? The value_type wraps a pair of the key and the data types in case of maps. And it wraps just the key in case of a set.
This means, in case of a map if we already have an entry with <K, D1> and then we add <K, D2> then a new tree is returned. In case of a set with an entry <K> and then we when we add <K> again the existing tree is returned.
What about this new wording?

// add_internal - Returns a tree that adds the specified  value to the
// original tree. A value is a <key,data> pair in case of a map, or a simple
// key in case of a set. If the original tree already contains an equal value,
// the original tree is returned.
// This means, in case of a map if we already have a value of <K, D1> and
// then we add <K, D2> then a new tree is returned.