it's not that I need this functionality; it's that your restrictions on the
behavior of the code introduce complexity into the class that I don't
understand the need for. The idea behind a builder is that it provides an
API for constructing an unchangeable object. How you intend to use it in
CopyOnWriteTransformer should not come into it, except insofar as it
provides the design constraints on the implementation. In this case, you
want to avoid unnecessary copying.

Simple code is better than complex code. This class as it currently stands
is quite complex and has a number of subtleties that you need to understand
about it, despite the fact that I don't perceive the advantage those
subtleties actually provide in achieving the design constraint, which is an
immutable BuiltinMap class that can be constructed with as little copying
as possible.

As far as minor issues go, there are most certainly minor issues that are
not worth arguing over in a code review. The architectural structure of a
class and unnecessary complexity in it are simply not one of them, though.
Ensuring that the code that gets committed has the right design is the
entire _purpose_ of a code review.


On Tue, Jun 3, 2014 at 10:39 AM, Yilong Li <notifications@github.com> wrote:

> In
> src/javasources/KTool/src/org/kframework/backend/java/kil/BuiltinMap.java:
>
> > +
> > +        public Term remove(Term key) {
> > +            return entries.remove(key);
> > +        }
> > +
> > +        public Map<Term, Term> getEntries() {
> > +            return UnmodifiableMap.decorate(entries);
> > +        }
> > +
> > +        /**
> > +         * Sets the entries without copying the contents. Once the entries are
> > +         * set, no more modification is allowed.
> > +         *
> > +         * @param map
> > +         */
> > +        public void setEntries(Map<Term, Term> map) {
>
> Because the way to build a BuiltinMap is driven by the use cases in the
> CopyOnWriteTransformer. We are not building a library for users; I don't
> think there is a point in arguing over this minor issue. If you really
> really need such functionality, I can do it for you.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/kframework/k/pull/537/files#r13341360>.
>