Skip to main content

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [List Home]
[eclipse-pmc] Review of mass changes

Dear PMC,

First topic.

I was asked by Lars to ask PMC to help with the review of the change
https://git.eclipse.org/r/#/c/144392/, especially

https://git.eclipse.org/r/#/c/144392/1/bundles/org.eclipse.osgi.compatibility.state/src/org/eclipse/osgi/internal/resolver/StateHelperImpl.java@262

The patch with the title "Use isEmpty instead of size" and the comment
"Improves readability" suggests to change

return getPossibleCandidates(constraint, exports, null, hook,
true).size() > 0;

to

return !getPossibleCandidates(constraint, exports, null, hook,
true).isEmpty();

In my experience such code where "not" is far away from the actual
negated  method call leads to misinterpretation of the code while
reading, and therefore does not improve the code readability as claimed
in the patch comment.

It looks like Lars has different opinion on this topic, so he asked me
to ask PMC about this.

I personally think that I do not need to ask PMC every time I comment on
some patch, but if Lars asks, I do, so here we are. I'm sorry for
wasting your time, but it was not my idea to call PMC.

Please review the patch as asked by Lars.

Second topic.

From my experience in the past years we had many mass-patches that
broke something, and I had pleasure to fix them by myself. In most cases
those mass patches were considered "harmless" and committers never asked
anybody else for a review and just merged the patches.

Now Lars started another huge patch series that are going across entire
Eclipse platform, where not just some white space is changed, but where
the real code (conditionals) are changed, and where very subtle bugs may
appear. I have no time (and I don't want) to review all those mass
changes, but someone should, because it is irresponsible to do something
like this in such way.

I would like to ask PMC here that from now on and for the future such
mass changes that change real code (no white space etc) must have a
mandatory +1 from a  second reviewer, to avoid regressions.

Thanks.

--
Kind regards,
Andrey Loskutov

https://www.eclipse.org/user/aloskutov
---------------------------------------------
Спасение утопающих - дело рук самих утопающих
---------------------------------------------


Back to the top