Allow custom @SuppressWarnings in JFlex 1.8.2

[tbl0605]

Hi,
I'm from eclipse PDT team, and we recently switched from JFlex 1.6.1 to JFlex 1.8.2, it works great, thank you for your hard work, it's much appreciated!
We just noticed a minor issue (if we can call this an issue) while using our lexical specification files.
Long time ago, we've added our own "@SuppressWarnings" annotations, so our lexical files start with:

package org.eclipse.php.internal.core.ast.scanner.php;
import ...;

@SuppressWarnings({"unused", "nls"})

%%
...

that will now be generated as:

// DO NOT EDIT
// Generated by JFlex 1.8.2 http://jflex.de/
...
package org.eclipse.php.internal.core.ast.scanner.php;
...
@SuppressWarnings({"unused", "nls"})


// See https://github.com/jflex-de/jflex/issues/222
@SuppressWarnings("FallThrough")
public class PHPAstLexer ...

The fact that JFlex 1.8.2 adds its own '@SuppressWarnings("FallThrough")' annotation will now result in java compilation errors (Duplicate annotation of non-repeatable type @SuppressWarnings.).
For now we simply removed our "@SuppressWarnings" annotations to workaround the issue.

This brings me to following question:
would it be possible to add in future releases a new JFlex run option (for example --annotations) to add our own annotations again?

Thank you!

[lsf37]

Thanks for reporting this. Would it make sense to make this part of the lexer spec, e.g. something like adding a %SuppressWarnings option?

[tbl0605]

Thanks for reporting this. Would it make sense to make this part of the lexer spec, e.g. something like adding a %SuppressWarnings option?

Any solution would be welcome, I leave it to your appreciation :)

[regisd]

Thanks for filing the issue. I also realised the problem it causes on the compilation in #453 (comment)

We have a few options

• Add @Generated (Add @Generated on generated code #453) which allows users to relax the error-prone warnings for this code ; and allows us to completely remove the @SuppressWarnings.
• Add %SuppressWarnings in the grammar which contains "FallThrough" by default

In the meantime, can you move the @SuppressWarnings on the methods that violate "nls" or "unused" (or are those also on generated code)?

[tbl0605]

In the meantime, can you move the @SuppressWarnings on the methods that violate "nls" or "unused" (or are those also on generated code)?

Yes, sure, we do that, it's the cleanest solution, we have (luckily) very few places were we need @SuppressWarnings annotations.
And @SuppressWarnings("nls") was actually only added to annote strings generated by JFlex, for code like

      throw new java.io.IOException(
          "Reader returned 0 characters. See JFlex examples/zero-reader for a workaround.");
    }

or for the ZZ_*_PACKED_* strings.

A workaround would be to adapt our own skeleton files by adding //$NON-NLS-1$

    if (numRead == 0) {
      throw new java.io.IOException(
          "Reader returned 0 characters. See JFlex examples/zero-reader for a workaround."); //$NON-NLS-1$
    }

but the general idea for us (by upgrading to JFlex 1.8.2) is to refactor our code to completely remove the need of custom skeletons ;)
Anyway, the "nls" stuff is just a detail, if I remember well it was added only because old Eclipse versions complained about that (it's no more the case).

NB: in Eclipse @SuppressWarnings("FallThrough") is marked as "Unsupported", so anyway we will never get rid of all warnings everywhere lol

[lsf37]

but the general idea for us (by upgrading to JFlex 1.8.2) is to refactor our code to completely remove the need of custom skeletons ;)

That'd be cool. Let us know if you hit any other issues!

Anyway, the "nls" stuff is just a detail, if I remember well it was added only because old Eclipse versions complained about that (it's no more the case).

I used Eclipse for JFlex development a while ago and still have quite a few of these //$NON-NLS-1$ comments around. The warnings did help initially to find the things that should be localised, but overall it did generate too much noise, yes.

NB: in Eclipse @SuppressWarnings("FallThrough") is marked as "Unsupported", so anyway we will never get rid of all warnings everywhere lol

Good to know, so if we do something like %SuppressWarnings we should also provide an option for removing it completely, not just for replacing the default. Or maybe @Generated will solve both.

[tbl0605]

Good to know, so if we do something like %SuppressWarnings we should also provide an option for removing it completely, not just for replacing the default. Or maybe @generated will solve both.

To resume all we discussed, the only "true" problem I see is that there should be some option to remove/disable @SuppressWarnings automatically added by JFlex. Forget about the "nls" stuff, I don't know if it's worth the headaches. On the other side, you are totally right, the user should only add its own @SuppressWarnings on user methods that really need them, it's globally a better coding practice, I'll do this way ;)

Anyway, thanx for taking so many time on this minor issue :)

[rmuir]

We hit this issue too with apache lucene :)

One practical problem with the current @SuppressWarnings("FallThrough") is that this isn't recognized by javac in this mixed-case form.

So we had to use find-replace and change it to @SuppressWarnings("fallthrough"), which works. Maybe the mixed-case is enough for error-prone and similar tools, but javac seems to be case-sensitive.

[lsf37]

Thanks for that, it's good to know that case sensitivity might be an issue as well.