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

 
 

patch #9067: java sources part 1

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

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( 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

java.lang.Byte.valueOf(byte)
java.lang.Double.valueOf(double)
java.lang.Float.valueOf(float)
java.lang.Integer.valueOf(int)
java.lang.Long.valueOf(long)
java.lang.Short.valueOf(short)

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

* file-name (function-name): What changed.

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

return Long.valueOf (num.longValue ());

instead of

return Long   .valueOf(num.  longValue());

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

  public static Object createArray (Object cls, int length)

and add some comments to method

  public static Object createArray (Object cls, int[] length)

The following would do:

 //! Return an array with entry type given by @c cls
  //! with dimensions given by @c dims.
  //! The class may be given directly as a @ref Class object
  //! or as a string via its name.
  //! Used by @file javaArray.m defining @c javaArray only.
  //!
  //! Note that if you invoke <code>javaArray('java.lang.Object', 1)</code>,
  //! this is translated into invocation of
  //! <code>ClassHelper.createArray("java.lang.Object", 1)</code>,
  //! invoking ::createArray(Object, int),
  //! whereas <code>javaArray('java.lang.Object', 1, 1)</code>
  //! this is translated into invocation of
  //! <code>ClassHelper.createArray("java.lang.Object", 1, 1)</code>,
  //! invoking ::createArray(Object, int[]).
  //!
  //! @param cls
  //!    the atomic component class given as a class object or as a string.
  //!    If given as a string, the class is loaded via #loader.
  //! @param dims
  //!    the dimensions which shall be non-negative but less than 256.
  //! @return
  //!    an array with the given component type and dimensions.
  //!    If @c cls is a string, the class is loaded by #loader.
  //! @throws ClassNotFoundException
  //!    if @c cls is a string representing a class name
  //!    and if #loader could not find the according class.
  //! @see ::createArray(Object, int)
  // based on loader but only partially.
  // used by javaArray.m
  public static Object createArray (Object cls, int[] dims)
    throws ClassNotFoundException
  {
    // FIXME: seems as if used in javaArray.m only.
    // Then more appropriate seems split into two methods:
    // one with cls is a Class other one with cls is a String
    // FIXME: if a class is given directly,
    // it is not guaranteed, that this is loaded via #loader
    // So this shall be fixed.
    // Typically, different usages are treated in m file invoking error
    // in the last else branch.
    // This would be appropriate here also.
    Class theClass;
    if (cls instanceof Class)
      {
        theClass = (Class) cls;
      }
    else if (cls instanceof String)
      {
        // may throw ClassNotFoundException
        theClass = Class.forName ((String) cls, true, loader);
      }
    else
      {
        throw new IllegalArgumentException ("invalid class specification " +
                                            cls);
      }

    return Array.newInstance (theClass, dims);
  }


  //! Return a one-dimensional array with entry type given by @c cls
  //! with length by @c length.
  //! The class may be given directly as a @ref Class object
  //! or as a string via its name.
  //! Used by @file javaArray.m defining @c javaArray only.
  //!
  //! @param cls
  //!    the atomic component class given as a class object or as a string.
  //!    If given as a string, the class is loaded via #loader.
  //! @param length
  //!    the length of the array to be created.
  //! @return
  //!    an array with the given component type and length.
  //!    If @c cls is a string, the class is loaded by #loader.
  //! @throws ClassNotFoundException
  //!    if @c cls is a string representing a class name
  //!    and if #loader could not find the according class.
  //! @see ::createArray(Object, int)
  // based on loader but only partially.
  // used by javaArray.m
  // would be superfluous with varargs introduced in java 1.5:
  // the createArray (Object cls, int[] length)
  // could be written createArray (Object cls, int... length)
  public static Object createArray (Object cls, int length)
    throws ClassNotFoundException
  {
    return createArray (cls, new int[] {length});
  }

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 ernstreissner (Voted in favor of this item)
  • -email is unavailable- added by ernstreissner (Submitted the item)
  • -email is unavailable- added by ernstreissner
  •  

    There is 1 vote so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

     

     

     

    Follow 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-07 mtmiller Carbon-CopyRemoved -email is unavailable- -
    2019-03-07 mtmiller Carbon-CopyRemoved 80942 -
    2019-03-07 mtmiller CategoryNone Core : other
    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.9