Code review request

[cushon]

Original issue created by sjnickerson@google.com on 2012-09-11 at 11:21 AM

---

Branch name: longliteral

Purpose of code changes on this branch: Add checker for long literals ending with lower case ell, e.g. 123432l rather than 123432L. See #23

After the review, I'll merge this branch into: /master

[cushon]

Original comment posted by alexeagle@google.com 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: eaftan@google.com

[cushon]

Original comment posted by sjnickerson@google.com 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: alexeagle@google.com

[cushon]

Original comment posted by sjnickerson@google.com 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.

[cushon]

Original comment posted by alexeagle@google.com 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.

[cushon]

Original comment posted by eaftan@google.com 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.

[cushon]

Original comment posted by eaftan@google.com on 2012-09-13 at 12:39 AM

---

(No comment entered for this change.)

---

Status: Fixed