patchGNU Octave - Patches: patch #9067, java sources part 1

 
 

patch #9067: java sources part 1

Submitted by:  Ernst Reissner <ernstreissner>
Submitted on:  Wed 03 Aug 2016 04:43:14 PM UTC  
Votes:  1  
 
Category:  None Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Sat 13 Aug 2016 03:58:45 AM UTC, comment #12:

As Ernst explained to me in another bug report, the Java class static methods

should be preferred to the normal constructor or auto-constructing a class wrapper from the primitive value. These static methods maintain a cache of frequently used values.

See https://docs.oracle.com/javase/7/docs/api/java/lang/Long.html#valueOf(long) for the official description, which says that this method should be preferred to calling the default constructor.

Mike Miller <mtmiller>
Project Administrator
Sat 13 Aug 2016 01:18:08 AM UTC, comment #11:

It's possible to use "hg histedit" to combine changesets.

If you can't do that, then are these supposed to be all applied as one changeset?

Please edit the commit message so that it has a one line summary followed by a blank line, then more detailed info about the file(s) and functions that were changed. Usually that should be in the form

though not all changes need a great amount of detail. If you are just fixing style issues, then a list of filenames followed by "Style fixes." is usually sufficient.

Please try to avoid mixing style changes with other substantive changes. It makes the substantive changes harder to evaluate.

Please write

instead of

Also, in this particular case, is the Long.valueOf () function actually needed?

John W. Eaton <jwe>
Project Administrator
Fri 12 Aug 2016 12:00:11 AM UTC, comment #10:

What I did is just reviewing the java sources
and making changes to improve quality, readability
and to provide docs.

Ernst Reissner <ernstreissner>
Thu 11 Aug 2016 11:56:57 PM UTC, comment #9:

I am sorry but hg export -r22201:22254:22259 -o patch9067B.patch did not work.

I provide individual patches instead
hg export -r22201 >... and so on..

Please help me providing an all inclusive patch...

(file #38182, file #38183, file #38184)

Ernst Reissner <ernstreissner>
Thu 11 Aug 2016 01:06:43 PM UTC, comment #8:

Hi Mike,
hope that works for you.
I am not a mercurial specialist,
but willing to learn.

Thank you for reviewing and tolerating stress.

(file #38175)

Ernst Reissner <ernstreissner>
Thu 11 Aug 2016 12:26:57 PM UTC, comment #7:

Hi Mike,
I tried not to make trailing spaces (as I am used to do).
Some places I found and corrected.
Now it should be correct.

Concerning indentation, now I applied my auto-indenter...
should be ok now.

Break of function arguments: I also corrected.

Now I deleted code that was commented out.

NOPMD: I use a coding style checker detecting weak coding,
called PMD.
Sometimes it is better to break rules,
but this must be documented.
NOPMD is a marker for PMD program to suspend checking.

In the new patch I replaced 'NOPMD'
by 'NOPMD: rulecheck suspended'
If this is not acceptable to you, please let me know.

The place with else if is probably e.g. in method setStaticField.
I think code is better now because I have problems
finding the if for the else... Hm..
maybe a matter of habit.
Maybe it is better because the then branch consists of nothing but the return.
I must insist, that the last if i deleted is an improvement.
;-)

What do you mean by 'please fold those changes into and refresh the existing patch'?

Shall I provide a patch with all changes?
You do not want a follow up patch with only the corrections to the path, right?

Concerning your last point:
I tried not to change functionality at all.
Just simplified and documented.

An exception is method addClassPath,
which is now based on OctClassLoader.addUrl,
no longer on OctClassLoader.addClassPath.
Thus I could remove this method, which is no longer required.

What concerns exceptions or more general Throwables,
I did two things:
I declared more precisely what kind of Exceptions could be thrown.
Throwable is the base class, subclasses are Exception and Error.
It is bad style just to declare Exception,
because this can be everything.
So I specified the subclass tht can be thrown as precisely as possible.
Also I documented the cases in which that kind of exception can be thrown.

An example is removeClassPath:
No longer Exception is thrown, but MalformedURLException.
Also it is declared that this cannot occur,
if the parameter path is really a path.
Also it is documented in the body of the method,
where exactly the exception could be thrown.

The second thing I did,
is that I generally eliminated empty catches,
because they are considered bad style hiding misbehavior.
This is the case for getClassPath().
Now the exception is not silently dropped,
but thrown.
In this case, I gave an explanation,
why this exception cannnot occur,
although declared.

I think, I can explain in all places,
that i did not change functionality.

If you have questions, please ask.

I have the impression, you do not feel so very comfortable
with my changes,
maybe because you do not have any tests at hand.
I could provide, but only for internal use,
an arithmetics defined in java using the interface.

Are you interested in that?

Answering your other post:
I fully understand your point, that precedence is:

reformat/redesign < documentation < feature < bugfix,

but sometimes
reformat/redesign and documentation prior to bugfix
eases review of bugfix considerably.

I hope this is the case here.

Ernst Reissner <ernstreissner>
Thu 11 Aug 2016 09:15:54 AM UTC, comment #6:

I took 5 minutes and reviewed very briefly. I can offer some basic code formatting suggestions.

  • Please delete all trailing whitespace at the end of every line.
  • Watch your indentation, I see at least one place that is indented by an extra space, around line 330 of ClassHelper.java.
  • Do not break function arguments on every comma, only break when the line exceeds 80 columns.
  • I don't know what these "PMD" and "NOPMD" comments mean.
  • Don't comment out code that should just be deleted, that's what hg is for.
  • Is it really so bad to keep "else if" after a case that does a return? The "else" at the start of the line makes it clear that the cases are related alternatives, otherwise you have to look into the if block to see whether it returns something or not.

If you want to make changes to your patch, please fold those changes into and refresh the existing patch.

I have no comments on the rest, it's Java that is over my head and too deep to review quickly. I have no idea if the changes you have made make sense, or are safe, or do the same thing that they did before. For example you change getClassPath so that it could throw an exception. Is this safe? I don't know. Someone else may, but I can't help review these types of Java functionality changes.

Mike Miller <mtmiller>
Project Administrator
Thu 11 Aug 2016 08:43:26 AM UTC, comment #5:

Please be patient. We do appreciate your interest in contributing to Octave's Java interface. I am not the only reviewer, we are all stretched thin and have plenty of work to do. I'm not saying it's not worth applying, only that I haven't had time to review this patch and probably won't soon. With the time that I have, I'm more likely to review a patch that fixes a bug than a patch that provides a new feature, and more likely to review either of those than a patch that reformats code.

Mike Miller <mtmiller>
Project Administrator
Thu 11 Aug 2016 07:34:05 AM UTC, comment #4:

Hi Mike,
Is there something wrong with the patch, that you do not consider it? But then you should tell me that.
How can we proceed??????

Ernst Reissner <ernstreissner>
Wed 10 Aug 2016 11:10:07 PM UTC, comment #3:

I need feedback to the way i wrote doxygen comments
and how I reimplemented parts of the java class.
Only if ok, I can go on with the remaining 3 java sources.

Ernst Reissner <ernstreissner>
Wed 10 Aug 2016 12:21:14 PM UTC, comment #2:

Let me patch the patch:

We have to reinsert method

and add some comments to method

The following would do:

Ernst Reissner <ernstreissner>
Tue 09 Aug 2016 09:33:09 AM UTC, comment #1:

as this is the startpoint of several bugfixes,
I would like to vote for this patch.

Ernst Reissner <ernstreissner>
Wed 03 Aug 2016 04:43:14 PM UTC, original submission:

Hi,
i just endowed part of sources with doxygen comments,
added comments where i see a bug,
simplified implementation (partially replaced recursion by loops),
and declared throwables more carefully.

Ernst Reissner <ernstreissner>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #38182:  r22201.patch added by ernstreissner (53KiB - text/x-patch)
file #38183:  r22259.patch added by ernstreissner (1KiB - text/x-patch)
file #38184:  r22254.patch added by ernstreissner (29KiB - text/x-patch)
file #38175:  patch9067B.patch added by ernstreissner (540KiB - text/x-patch)
file #38094:  patchJavaSrc.patch added by ernstreissner (53KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by ernstreissner (I would like to fix this before i do further work)
  • -email is unavailable- added by ernstreissner (Voted in favor of this item)
  • -email is unavailable- added by ernstreissner (Submitted the item)
  • -email is unavailable- added by ernstreissner
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 1 encouragement so far.

    Only logged-in users can vote.

     

     

     

    Follow 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-11 ernstreissner Attached File- => Added r22201.patch, #38182
        Attached File- => Added r22259.patch, #38183
        Attached File- => Added r22254.patch, #38184
    2016-08-11 ernstreissner Attached File- => Added patch9067B.patch, #38175
    2016-08-10 ernstreissner Carbon-Copy- => Added -email is unavailable-
    2016-08-09 ernstreissner Carbon-Copy- => Added ernstreissner
    2016-08-03 ernstreissner Attached File- => Added patchJavaSrc.patch, #38094
        Carbon-Copy- => Added -email is unavailable-

    Back to the top


    Powered by Savane 3.3