Community
Participate
Working Groups
I20050802-0800 In CoolBarManager (org.eclipse.jface) there are NLS tags inside of commented-out blocks of code. These are being reported as compiler errors (when "Usage of non-externalized strings" is set to error). NLS tags inside of commented-out sections should be ignored. If the commented-out sections were uncommented, then the NLS tags would not be errors.
There is no way to make the distinction between the ones in commented code and the others. The position of the non-nls tags is unspecified, therefore they could be anywhere on the line. Move to JDT/Core.
I think it will be possible to detect this case. A NLS tag is identifiable by the pattern: '//[:white]*$NON-NLS-1$'. If the NLS tag is preceded by a '//' which is not an NLS tag, then the line is commented out. I'm not sure how the code works to detect multi-line comments, but I assume something similar should be possible. This problem is rather severe: commenting out source code should not lead to errors.
As I said, it is possible right now to put the nls tag anywhere within the line comment. So your suggestion won't work. If nls tags were specified to start the line comment, then yes we could handle this case. Unfortunately this is unspecified and changing it could break existing code base. There is no way to handle this case due to the unspecified nature of the nls tags. The provided tool will remove these tags inside line comments.
So, you saying that the specification for NLS tags would allow NLS tags such as: "// This is an NLS tag $NON-NLS-1$" Where is the syntax for NLS tags specified?
Just to clarify. I'm not overly concerned about the one-time hit of removing NLS tags that the compiler detects as extraneous inside of comments. I'm worried about the use case of Platform/UI developers commented out code in the future. The way things are in I20050802-0800, if I comment out a section of code with an NLS tag in it, the commented out section appears with a compiler error.
(In reply to comment #4) > So, you saying that the specification for NLS tags would allow NLS tags such > as: > "// This is an NLS tag $NON-NLS-1$" > Where is the syntax for NLS tags specified? This is forbidden, but this is fine: "// This is an NLS tag //$NON-NLS-1$" Even if this is rare, this is not impossible.
(In reply to comment #5) > Just to clarify. I'm not overly concerned about the one-time hit of removing > NLS tags that the compiler detects as extraneous inside of comments. I'm > worried about the use case of Platform/UI developers commented out code in the > future. The way things are in I20050802-0800, if I comment out a section of > code with an NLS tag in it, the commented out section appears with a compiler > error. Use a block comment and you won't have the problem. This is something specific to the format of nls tags.
So that particular pattern could either be interpreted as an NLS tag or as a commented out block of code? While I don't want you guys to design the NLS mechanism purely for how Platform/UI has been developing code, I would think that other people have set the "Usage of non-externalized strings" preference to error. It's an easy way to catch externalization errors incrementally. However, this also means that when we comment out code, we'd have to remove NLS tags -- reinserted them when the code is uncommented. If the pattern is infrequently an NLS tag and frequently a commented out section of code, then perhaps it would be better to not report the problem in this case.
(In reply to comment #7) > Use a block comment and you won't have the problem. This is something > specific to the format of nls tags. This is true, but "Ctrl+Shift+C" does not do a block comment.
I see no problem at all in specifying how a nls tag should appear in the code. It would simplify my life if the nls tag has to start the line comment. In case of multiple nls tag on the same line, the first one would need to start the line comment. And a nls tag could not be used to add extra comments. Something like: //$NON-NLS-1$ this is a comment would be illegal. I would be glad to follow this description. Unfortunately, this would not be backward compatible. Because of the backward compatibility case, I don't see a good way to solve this issue. Mike, any suggestion?
(In reply to comment #8) > However, this also means that when we comment out code, we'd have to remove > NLS tags -- reinserted them when the code is uncommented. Just an idea, can the comment/uncomment action (CTRL+SHIFT+C) be modifier to handle this case automatically? > If the pattern is infrequently an NLS tag and frequently a commented out > section of code, then perhaps it would be better to not report the problem in > this case. However, I also agree that unnecessary NLS tags inside commented blocks should not be reported. BTW, I never thought that the following syntax is valid (from the NLS tooling point of view): //$NON-NLS-1$ this is a comment Can we check if there is some code before the comment or if the single-line comment (//...) is in an empty line?
I think we agree that we need to specify how the nls tag should be used. We have 2 differents cases: - add nls tags on a line without comment: This case is trivial to handle - add nls tags on a line that already ends with a line comment Should we add it at the end, at the beginning? Having an NLS tag that would be a block comment like this /*$NON-NLS-n$*/ would help getting these cases right, because they don't have a scope that goes to the end of line.
CCing Dani, since he is the NLS tooling expert. My two cents: IMO cases like "// some comment // $NON-NLS-1$" or "// $NON-NLS-1$ some comment" are absolutely rare (I would even say they don't show up at all at least in Eclipse code. So I would opt for the following: in cases where an $NON-NLS-1$ comment appears in the range of another comment and this other comment isn't an NLS comments as well I would consider the NLS tag to be commented out and would ignore it when analysing unnecessary NLS tags. The failure case is then that we don't warn about NLS tags which are indeed unnecessary.
I'm not sure why this is hard... It seems like you could just inspect the comment string contents: If it consists entirely of the comment indicator and N occurrences of "$NON-NLS-x$", then remove it completely. If there are other (non-whitespace) characters present, just remove all the "$NON-NLS-x$" tags and leave the rest. Won't that solve the problem?
(In reply to comment #13) > IMO cases like "// some comment // $NON-NLS-1$" or "// $NON-NLS-1$ some comment" > are absolutely rare (I would even say they don't show up at all at least in > Eclipse code. Dirk, this is more frequent than you think. If you have a line comment on the same line than a string literal, the quickfix adds the //$NON-NLS-n$ at the end of the line which means that we end up with: // my comment //$NON-NLS-1$ So this needs to be clarified. If this is not allowed we should fine a way for the quick fix to work.
I guess this case could be handled if I consider that I should not report unnecessary NLS tags within a line comment that starts on column 0. When you use Ctrl + Shift + C, the line comments always starts on column 0. If this is the case, there is nothing to report, otherwise I would report unnecessary NLS tags. So as long as you leave your commented out code on column 0, you should be fine. This is a hack, but I think it handles this case pretty well.
In my opinion it's quite simple: if you have a line like this: 0..n whitespace + // then it is a traditional comment according to the JLS and no warnings should be generated.
To keep it simple, I don't check for whitespaces. I can add that check, but this would slow down the detection.
The check needs to be added otherwise it's just not correct.
Done. I can provide a binary patch of jdt/core and a tool that removes all superflous nls tags to who wants it.
Olivier: Can you post the tool and the patch to the JDT/Core web site?
The binary patch and the tool are available on the JDT/Core page. The patch is there: http://dev.eclipse.org/viewcvs/index.cgi/%7Echeckout%7E/jdt-core-home/r3.2/main.html#updates The tool is there: http://dev.eclipse.org/viewcvs/index.cgi/%7Echeckout%7E/jdt-core-home/tools/jdtcoretools/index.html Please let me know if you find anything wrong.
(In reply to comment #16) > When you use Ctrl + Shift + C, the line comments always starts on column 0. This is correct but as soon as you format your source code the entire block is alligned with the source code above/below. You comment now doesn't start at the column 0 any more. Thus, what I usually do is: - select some lines to comment out - press Shift+Tab until the block is at column 0 - press Ctrl + Shift + C - format the source
Don't worry, Gunnar. With the provided patches, lines that contains only a line comment are not checked for unnecessary nls tags.
This is fixed in HEAD. A tool that removes automatically all unnecessary tags is available at: http://dev.eclipse.org/viewcvs/index.cgi/%7Echeckout%7E/jdt-core-home/tools/jdtcoretools/update-site This tool requires a build > 20050812. Regression test added in org.eclipse.jdt.core.tests.compiler.regression.ExternalizeStringLiteralsTest.test001/004
Verified using I20050920-0010 for 3.2M2
