commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benedikt Ritter <brit...@apache.org>
Subject 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)
Date Tue, 07 May 2013 07:01:57 GMT
2013/5/6 sebb <sebbaz@gmail.com>

> On 6 May 2013 18:53, Benedikt Ritter <britter@apache.org> wrote:
>
> > 2013/5/5 sebb <sebbaz@gmail.com>
> >
> > > On 3 May 2013 14:37, Benedikt Ritter <britter@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 <sebbaz@gmail.com>
> > > >
> > > > > On 3 May 2013 07:40, Benedikt Ritter <britter@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 <britter@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

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message