[PATCH] D106367: [ADT][NFC] Correct the wrong header comment of ImmutableSet::add_internal

[Gabor Marton via Phabricator via llvm-commits]

martong added inline comments.


================
Comment at: llvm/include/llvm/ADT/ImmutableSet.h:541-542
   /// add_internal - Creates a new tree that includes the specified
   ///  data and the data from the original tree.  If the original tree
-  ///  already contained the data item, the original tree is returned.
+  ///  already contained the data item, a new tree is returned.
   TreeTy* add_internal(value_type_ref V, TreeTy* T) {
----------------
vsavchenko wrote:
> 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).
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.
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D106367/new/

https://reviews.llvm.org/D106367