You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Benedikt Ritter <br...@apache.org> on 2013/05/03 15:37:06 UTC
[CSV] Move isXXX(int) Methods from Lexer to CSVFormat? (was: Re: svn
commit: r1478655 - /commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java)
(moving this to a new thread)
Hi,
I did a very small refactoring in Lexer just to realize that the isXXX(int)
methods (like boolean isDelimiter(int c)) really belong to CSVFormat rather
than to the Lexer. How do you feel about this?
Benedikt
2013/5/3 sebb <se...@gmail.com>
> On 3 May 2013 07:40, Benedikt Ritter <br...@apache.org> wrote:
>
> > The format field in Lexer is now only used by CSVLexer1 in the test
> > directory. It may therefore be removed.
> > I wonder if the isXXX methods really belong to CSVFormat.
> >
> > WDYT?
> >
> >
> The CSVLexerN entries in the test directory are copies of the originals,
> for comparative performance tests, and should be left alone as far as
> possible.
> Benedikt
>
> >
> >
> > 2013/5/3 <br...@apache.org>
> >
> > > Author: britter
> > > Date: Fri May 3 06:37:58 2013
> > > New Revision: 1478655
> > >
> > > URL: http://svn.apache.org/r1478655
> > > Log:
> > > Use isDelimiter method instead of != check.
> > >
> > > Modified:
> > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > >
> > > Modified:
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > URL:
> > >
> >
> http://svn.apache.org/viewvc/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java?rev=1478655&r1=1478654&r2=1478655&view=diff
> > >
> > >
> >
> ==============================================================================
> > > ---
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > (original)
> > > +++
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > Fri May 3 06:37:58 2013
> > > @@ -146,7 +146,7 @@ abstract class Lexer {
> > > * @return true if the given char is a whitespace character
> > > */
> > > boolean isWhitespace(final int c) {
> > > - return c != format.getDelimiter() &&
> > > Character.isWhitespace((char) c);
> > > + return !isDelimiter(c) && Character.isWhitespace((char) c);
> > > }
> > >
> > > /**
> > >
> > >
> > >
> >
> >
> > --
> > http://people.apache.org/~britter/
> > http://www.systemoutprintln.de/
> > http://twitter.com/BenediktRitter
> > http://github.com/britter
> >
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: [CSV] Move isXXX(int) Methods from Lexer to CSVFormat? (was: Re:
svn commit: r1478655 - /commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java)
Posted by Benedikt Ritter <br...@apache.org>.
2013/5/6 sebb <se...@gmail.com>
> On 6 May 2013 18:53, Benedikt Ritter <br...@apache.org> wrote:
>
> > 2013/5/5 sebb <se...@gmail.com>
> >
> > > On 3 May 2013 14:37, Benedikt Ritter <br...@apache.org> wrote:
> > >
> > > > (moving this to a new thread)
> > > >
> > > > Hi,
> > > >
> > > > I did a very small refactoring in Lexer just to realize that the
> > > isXXX(int)
> > > > methods (like boolean isDelimiter(int c)) really belong to CSVFormat
> > > rather
> > > > than to the Lexer. How do you feel about this?
> > > >
> > > >
> > > If I remember correctly, the methods were added to Lexer to improve
> > > performance.
> > >
> >
> > Oh, I never thought that calling a members method would have such an
> impact
> > on performance. Thanks for the info!
> >
> >
> I don't think it had a huge impact, but there would have been some benefit.
>
> Note also that the CSVFormat fields are Character rather than char, so the
> checks would be more complicated if moved to CSVFormat as it is currently.
>
> I don't remember if this was the case at the time the methods were moved.
>
Okay, this makes sense to me then.
Thanks for the clarification.
>
>
> >
> > >
> > >
> > > > Benedikt
> > > >
> > > > 2013/5/3 sebb <se...@gmail.com>
> > > >
> > > > > On 3 May 2013 07:40, Benedikt Ritter <br...@apache.org> wrote:
> > > > >
> > > > > > The format field in Lexer is now only used by CSVLexer1 in the
> test
> > > > > > directory. It may therefore be removed.
> > > > > > I wonder if the isXXX methods really belong to CSVFormat.
> > > > > >
> > > > > > WDYT?
> > > > > >
> > > > > >
> > > > > The CSVLexerN entries in the test directory are copies of the
> > > originals,
> > > > > for comparative performance tests, and should be left alone as far
> as
> > > > > possible.
> > > >
> > > >
> > > > > Benedikt
> > > > >
> > > > > >
> > > > > >
> > > > > > 2013/5/3 <br...@apache.org>
> > > > > >
> > > > > > > Author: britter
> > > > > > > Date: Fri May 3 06:37:58 2013
> > > > > > > New Revision: 1478655
> > > > > > >
> > > > > > > URL: http://svn.apache.org/r1478655
> > > > > > > Log:
> > > > > > > Use isDelimiter method instead of != check.
> > > > > > >
> > > > > > > Modified:
> > > > > > >
> > > > > > >
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > > >
> > > > > > > Modified:
> > > > > > >
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > > > URL:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> http://svn.apache.org/viewvc/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java?rev=1478655&r1=1478654&r2=1478655&view=diff
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> ==============================================================================
> > > > > > > ---
> > > > > > >
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > > > (original)
> > > > > > > +++
> > > > > > >
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > > > Fri May 3 06:37:58 2013
> > > > > > > @@ -146,7 +146,7 @@ abstract class Lexer {
> > > > > > > * @return true if the given char is a whitespace
> character
> > > > > > > */
> > > > > > > boolean isWhitespace(final int c) {
> > > > > > > - return c != format.getDelimiter() &&
> > > > > > > Character.isWhitespace((char) c);
> > > > > > > + return !isDelimiter(c) &&
> Character.isWhitespace((char)
> > > c);
> > > > > > > }
> > > > > > >
> > > > > > > /**
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > http://people.apache.org/~britter/
> > > > > > http://www.systemoutprintln.de/
> > > > > > http://twitter.com/BenediktRitter
> > > > > > http://github.com/britter
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > http://people.apache.org/~britter/
> > > > http://www.systemoutprintln.de/
> > > > http://twitter.com/BenediktRitter
> > > > http://github.com/britter
> > > >
> > >
> >
> >
> >
> > --
> > http://people.apache.org/~britter/
> > http://www.systemoutprintln.de/
> > http://twitter.com/BenediktRitter
> > http://github.com/britter
> >
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: [CSV] Move isXXX(int) Methods from Lexer to CSVFormat? (was: Re:
svn commit: r1478655 - /commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java)
Posted by sebb <se...@gmail.com>.
On 6 May 2013 18:53, Benedikt Ritter <br...@apache.org> wrote:
> 2013/5/5 sebb <se...@gmail.com>
>
> > On 3 May 2013 14:37, Benedikt Ritter <br...@apache.org> wrote:
> >
> > > (moving this to a new thread)
> > >
> > > Hi,
> > >
> > > I did a very small refactoring in Lexer just to realize that the
> > isXXX(int)
> > > methods (like boolean isDelimiter(int c)) really belong to CSVFormat
> > rather
> > > than to the Lexer. How do you feel about this?
> > >
> > >
> > If I remember correctly, the methods were added to Lexer to improve
> > performance.
> >
>
> Oh, I never thought that calling a members method would have such an impact
> on performance. Thanks for the info!
>
>
I don't think it had a huge impact, but there would have been some benefit.
Note also that the CSVFormat fields are Character rather than char, so the
checks would be more complicated if moved to CSVFormat as it is currently.
I don't remember if this was the case at the time the methods were moved.
>
> >
> >
> > > Benedikt
> > >
> > > 2013/5/3 sebb <se...@gmail.com>
> > >
> > > > On 3 May 2013 07:40, Benedikt Ritter <br...@apache.org> wrote:
> > > >
> > > > > The format field in Lexer is now only used by CSVLexer1 in the test
> > > > > directory. It may therefore be removed.
> > > > > I wonder if the isXXX methods really belong to CSVFormat.
> > > > >
> > > > > WDYT?
> > > > >
> > > > >
> > > > The CSVLexerN entries in the test directory are copies of the
> > originals,
> > > > for comparative performance tests, and should be left alone as far as
> > > > possible.
> > >
> > >
> > > > Benedikt
> > > >
> > > > >
> > > > >
> > > > > 2013/5/3 <br...@apache.org>
> > > > >
> > > > > > Author: britter
> > > > > > Date: Fri May 3 06:37:58 2013
> > > > > > New Revision: 1478655
> > > > > >
> > > > > > URL: http://svn.apache.org/r1478655
> > > > > > Log:
> > > > > > Use isDelimiter method instead of != check.
> > > > > >
> > > > > > Modified:
> > > > > >
> > > > > >
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > >
> > > > > > Modified:
> > > > > >
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > > URL:
> > > > > >
> > > > >
> > > >
> > >
> >
> http://svn.apache.org/viewvc/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java?rev=1478655&r1=1478654&r2=1478655&view=diff
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> ==============================================================================
> > > > > > ---
> > > > > >
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > > (original)
> > > > > > +++
> > > > > >
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > > Fri May 3 06:37:58 2013
> > > > > > @@ -146,7 +146,7 @@ abstract class Lexer {
> > > > > > * @return true if the given char is a whitespace character
> > > > > > */
> > > > > > boolean isWhitespace(final int c) {
> > > > > > - return c != format.getDelimiter() &&
> > > > > > Character.isWhitespace((char) c);
> > > > > > + return !isDelimiter(c) && Character.isWhitespace((char)
> > c);
> > > > > > }
> > > > > >
> > > > > > /**
> > > > > >
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > http://people.apache.org/~britter/
> > > > > http://www.systemoutprintln.de/
> > > > > http://twitter.com/BenediktRitter
> > > > > http://github.com/britter
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > http://people.apache.org/~britter/
> > > http://www.systemoutprintln.de/
> > > http://twitter.com/BenediktRitter
> > > http://github.com/britter
> > >
> >
>
>
>
> --
> http://people.apache.org/~britter/
> http://www.systemoutprintln.de/
> http://twitter.com/BenediktRitter
> http://github.com/britter
>
Re: [CSV] Move isXXX(int) Methods from Lexer to CSVFormat? (was: Re:
svn commit: r1478655 - /commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java)
Posted by Benedikt Ritter <br...@apache.org>.
2013/5/5 sebb <se...@gmail.com>
> On 3 May 2013 14:37, Benedikt Ritter <br...@apache.org> wrote:
>
> > (moving this to a new thread)
> >
> > Hi,
> >
> > I did a very small refactoring in Lexer just to realize that the
> isXXX(int)
> > methods (like boolean isDelimiter(int c)) really belong to CSVFormat
> rather
> > than to the Lexer. How do you feel about this?
> >
> >
> If I remember correctly, the methods were added to Lexer to improve
> performance.
>
Oh, I never thought that calling a members method would have such an impact
on performance. Thanks for the info!
>
>
> > Benedikt
> >
> > 2013/5/3 sebb <se...@gmail.com>
> >
> > > On 3 May 2013 07:40, Benedikt Ritter <br...@apache.org> wrote:
> > >
> > > > The format field in Lexer is now only used by CSVLexer1 in the test
> > > > directory. It may therefore be removed.
> > > > I wonder if the isXXX methods really belong to CSVFormat.
> > > >
> > > > WDYT?
> > > >
> > > >
> > > The CSVLexerN entries in the test directory are copies of the
> originals,
> > > for comparative performance tests, and should be left alone as far as
> > > possible.
> >
> >
> > > Benedikt
> > >
> > > >
> > > >
> > > > 2013/5/3 <br...@apache.org>
> > > >
> > > > > Author: britter
> > > > > Date: Fri May 3 06:37:58 2013
> > > > > New Revision: 1478655
> > > > >
> > > > > URL: http://svn.apache.org/r1478655
> > > > > Log:
> > > > > Use isDelimiter method instead of != check.
> > > > >
> > > > > Modified:
> > > > >
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > >
> > > > > Modified:
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > URL:
> > > > >
> > > >
> > >
> >
> http://svn.apache.org/viewvc/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java?rev=1478655&r1=1478654&r2=1478655&view=diff
> > > > >
> > > > >
> > > >
> > >
> >
> ==============================================================================
> > > > > ---
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > (original)
> > > > > +++
> > > > >
> > >
> commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > > Fri May 3 06:37:58 2013
> > > > > @@ -146,7 +146,7 @@ abstract class Lexer {
> > > > > * @return true if the given char is a whitespace character
> > > > > */
> > > > > boolean isWhitespace(final int c) {
> > > > > - return c != format.getDelimiter() &&
> > > > > Character.isWhitespace((char) c);
> > > > > + return !isDelimiter(c) && Character.isWhitespace((char)
> c);
> > > > > }
> > > > >
> > > > > /**
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > http://people.apache.org/~britter/
> > > > http://www.systemoutprintln.de/
> > > > http://twitter.com/BenediktRitter
> > > > http://github.com/britter
> > > >
> > >
> >
> >
> >
> > --
> > http://people.apache.org/~britter/
> > http://www.systemoutprintln.de/
> > http://twitter.com/BenediktRitter
> > http://github.com/britter
> >
>
--
http://people.apache.org/~britter/
http://www.systemoutprintln.de/
http://twitter.com/BenediktRitter
http://github.com/britter
Re: [CSV] Move isXXX(int) Methods from Lexer to CSVFormat? (was: Re:
svn commit: r1478655 - /commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java)
Posted by sebb <se...@gmail.com>.
On 3 May 2013 14:37, Benedikt Ritter <br...@apache.org> wrote:
> (moving this to a new thread)
>
> Hi,
>
> I did a very small refactoring in Lexer just to realize that the isXXX(int)
> methods (like boolean isDelimiter(int c)) really belong to CSVFormat rather
> than to the Lexer. How do you feel about this?
>
>
If I remember correctly, the methods were added to Lexer to improve
performance.
> Benedikt
>
> 2013/5/3 sebb <se...@gmail.com>
>
> > On 3 May 2013 07:40, Benedikt Ritter <br...@apache.org> wrote:
> >
> > > The format field in Lexer is now only used by CSVLexer1 in the test
> > > directory. It may therefore be removed.
> > > I wonder if the isXXX methods really belong to CSVFormat.
> > >
> > > WDYT?
> > >
> > >
> > The CSVLexerN entries in the test directory are copies of the originals,
> > for comparative performance tests, and should be left alone as far as
> > possible.
>
>
> > Benedikt
> >
> > >
> > >
> > > 2013/5/3 <br...@apache.org>
> > >
> > > > Author: britter
> > > > Date: Fri May 3 06:37:58 2013
> > > > New Revision: 1478655
> > > >
> > > > URL: http://svn.apache.org/r1478655
> > > > Log:
> > > > Use isDelimiter method instead of != check.
> > > >
> > > > Modified:
> > > >
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > >
> > > > Modified:
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > URL:
> > > >
> > >
> >
> http://svn.apache.org/viewvc/commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java?rev=1478655&r1=1478654&r2=1478655&view=diff
> > > >
> > > >
> > >
> >
> ==============================================================================
> > > > ---
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > (original)
> > > > +++
> > > >
> > commons/proper/csv/trunk/src/main/java/org/apache/commons/csv/Lexer.java
> > > > Fri May 3 06:37:58 2013
> > > > @@ -146,7 +146,7 @@ abstract class Lexer {
> > > > * @return true if the given char is a whitespace character
> > > > */
> > > > boolean isWhitespace(final int c) {
> > > > - return c != format.getDelimiter() &&
> > > > Character.isWhitespace((char) c);
> > > > + return !isDelimiter(c) && Character.isWhitespace((char) c);
> > > > }
> > > >
> > > > /**
> > > >
> > > >
> > > >
> > >
> > >
> > > --
> > > http://people.apache.org/~britter/
> > > http://www.systemoutprintln.de/
> > > http://twitter.com/BenediktRitter
> > > http://github.com/britter
> > >
> >
>
>
>
> --
> http://people.apache.org/~britter/
> http://www.systemoutprintln.de/
> http://twitter.com/BenediktRitter
> http://github.com/britter
>