Code review request about error-prone HOT 6 CLOSED

Original comment posted by [email protected] on 2012-09-11 at 09:06 PM

Shuffling to Eddie since I don't have time to take a careful look.
I'm quite concerned about the performance of re-reading the source file in the matches() method. We'll have to re-read any file containing a long literal, possibly many times. If we go this way, we'll need to performance profile and probably cache something.
Also, make sure your test case has some unicode characters before the matching literal, so char offset and byte offset can't be confused.
It would be a lot better if we can find some sneaky way to access this without going back to the source, though I bet the information is totally gone after parsing.

Owner: [email protected]

from error-prone.

Original comment posted by [email protected] on 2012-09-12 at 07:03 AM

Thanks for taking a look, Alex.

I share your concern, but looking at the object in the AST, it simply doesn't have the information needed. The JCLiteral object just knows its position in the source file, and its value as a long. It doesn't record know whether a suffix is present, let alone whether it's an 'L' or 'l'. It also don't record whether it's encoded as hex, or octal, or decimal, or whether there are underscores present (for Java 7) and whether the hex elements (a-f and the 'x' in 0x...) of it are upper or lower case. Unfortunately we need to somehow extract this from the source.

My belief (based on little evidence at the moment) is that the source code string is already probably cached - I assume this is why sourceFile.getCharContent() can return null. If this is indeed the case, would your objection go away?

CC: [email protected]

from error-prone.

Original comment posted by [email protected] on 2012-09-12 at 07:36 AM

I've tried this out by running "mvn test".

The call SourceFile#getCharContent() is actually RegularFileObject#getCharContent(), which includes this at the top of the method:

   CharBuffer cb = fileManager.getCachedContent(this);

Based on stepping through the code, this returns a non-null reference containing the cached source file data. This seems to indicate to me that this matcher isn't going to have to keep re-reading the source file.

from error-prone.

Original comment posted by [email protected] on 2012-09-12 at 06:10 PM

Yeah if the source is already cached, that's great. I imagined it might be, since the API gives you the CharSequence directly.
We'll still give it a performance check at some point before turning on generally. but I think it's a good check to enforce. It's never a bug, but it's trivial to fix and important for readability.

from error-prone.

Original comment posted by [email protected] on 2012-09-12 at 09:57 PM

FYI, JLS section 3.10.1 says of long literals, "The suffix L is preferred, because the letter l (ell) is often hard to distinguish from the digit 1 (one)." So I think we're justified in making this a compile error as long as it doesn't kill performance.

from error-prone.

Original comment posted by [email protected] on 2012-09-13 at 12:39 AM

(No comment entered for this change.)

Status: Fixed

from error-prone.