@IntDef in Java code - switch to it entirely, in parts, or avoid?

[Egor Pasko]

// Sorry for X-post, I noticed java@chromium is not very active.

Folks,

Marcin Wiacek (CCed) provides a nice justification [1] to switch to @IntDef. He is sending neat patches to switch to @IntDef part by part. One worry I have is that some OWNERs would be opposed to the idea of migrating to @IntDef for good reasons and we would end up in the migration stalled half-way, leaving the code base somewhat in inconsistent state.

What do people think about @IntDef in general? Are there any caveats?

Should we add @IntDef recommendation into the style guide [2]?

[1] Some justification about @IntDef from Marcin, with nice links:

https://chromium-review.googlesource.com/c/chromium/src/+/1133984

[2] Chromium Java style guide:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/java/java.md

[Andrew Grieve]

Style guide update sgtm. I can do this if we reach consensus here.

I'm not aware of any caveats.

--
You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To post to this group, send email to ja...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAH3q7_m0%2B23bTypoDuMNNUYJY75p-O4NeaCtHHYz8TddZuJ8dw%40mail.gmail.com.

[Yaron Friedman]

My apologies, I probably should've sent this to java@ earlier as I was reviewing the first few CLs. I like the change overall and would be supportive of making it part of the style guide.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1XM_nYy5-DFp44SUYFLZbWzk_c15VHjrJPT4qUUhCaOQg%40mail.gmail.com.

[Tommy Nyquist]

Yes, I also believe that we should update the style guide.

@IntDef is purely better than plain integers, since they can be checked at build time and enforced by lint. They also do not contribute to binary size because they are compiled out.

When comparing to enums, @IntDefs are much smaller, which is nice.

I guess one legitimate-ish reason to keep enums around is if you have a lot of logic around them such as fancy conversions, etc. But I don't think that's a common occurrence. Like the link from Marcin says, enums add ~1.0 to 1.4KB to our DEX files.

If it's just part of a "mode" or something like that for a method, an @IntDef should be enough. Readability is also typically OK as long as you use well named constants.

Core libraries like Jetpack more or less exclusively use @IntDef, so there's definitely precedence. I also chatted with someone from the Android API council about this, and they also thought it was a good idea for Chrome to move in this direction.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CAC6Qbv8N5oXq91PC%3D%3DZkOB_vfv%2BgJEErOTF7OZDYrRBBT5b99w%40mail.gmail.com.

[Donn Denman]

I agree that @IntDef is a net plus, but it appears that we do lose some type information when using them in Map and similar structs that require Integer instead of int.  If anyone knows of a workaround for this limitation that would be great.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAA6XDjOX3O%3DAM0vp5ZZ9BM%3DyZSHEtmwwNNEnDuUfPyhBECYBcQ%40mail.gmail.com.

[Tommy Nyquist]

So, in general I find it easier to provide a better review when developers split their patches into smaller parts. This could be per feature or per directory for example. This typically leads to less merge conflicts for you as a developer as well, since the CLs are generally quicker to review and land.

On Tue, Jul 17, 2018 at 12:34 PM Marcin Wiacek <marcin...@mwiacek.com> wrote:

Hi,

 

Thx for your mail.

 

There is general patch from this series in queue now (unfortunately XL), which is improving all/almost all EXISTING IntDef and making them really consistent across dirs. I will be more than happy when somebody could help me with fast landing it – it’s big and needs maintaining already.

 

https://chromium-review.googlesource.com/c/chromium/src/+/1137217

 

maybe Yaron? (please, next one will be definitely smaller)

 

With kind regards,

Marcin

--

You received this message because you are subscribed to the Google Groups "java" group.
To unsubscribe from this group and stop receiving emails from it, send an email to java+uns...@chromium.org.
To post to this group, send email to ja...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/000701d41e05%2433719ae0%249a54d0a0%24%40mwiacek.com.

[Tommy Nyquist]

So in general we'd like the discussion and consensus to happen on chromium-dev@ or java@ before we start making these types of changes that produce a lot of churn. There's a cost associated to not be able to quickly identify an author of a feature using git blame for example, since there's an extra step of indirection. In general, there has to be very compelling reasons to make changes over large swaths of code like this.

I don't think there's a benefit for reviewers to be able to review one big CL with many types of changes interleaved versus small and targeted changes.

For your specific change by the way, have you run tests to verify that you do not increase the number of methods or classes by introducing these interfaces?

On Tue, Jul 17, 2018 at 12:47 PM Marcin Wiacek <marcin...@mwiacek.com> wrote:

You’re right, but this case is different:

 

From CL description:

“The main purpose of it it moving all definition of existing @IntDef inside related interface - it helps with code readiness and gives a lot of other profits (understanding, which value belongs to each IntDef, etc.)

If possible, please do not drop this CL because it's big:

1. I have experience, that code owners want rather open discussion than just simply help in getting the same style around many dirs (…)

2. changes are even more than trivial and we could avoid many pain just by submitting one CL

3. people are on holidays

4. it's so simple that review for it will need no more than 0.5h “

See especially on point 4, one CL and we have consistency across tree, I will maintain it, but need review.

[Tommy Nyquist]

Hi Marcin,

I was specifically curious about the addition of the @Interface definition, and whether that lead to generation of interfaces that end up in our prod code?

On Tue, Jul 17, 2018 at 1:24 PM Marcin Wiacek <marcin...@mwiacek.com> wrote:

Hi Tommy,

 

I believe, that IntDef are one of these changes, which should be done automatically by developers on some level.

 

Please look very carefully and deeply into https://chromium-review.googlesource.com/c/chromium/src/+/1137217 before we will continue discussion – these IntDefs are already in code, I’m just improving their readability (this is like renaming a to A) + changes in many files are just for one line.

 

Other changes from these series are small and I totally agree with your arguments.

 

We’re using RetentionPolicy = SOURCE and we have already info https://developer.android.com/topic/performance/reduce-apk-size#remove-enums, do you expect numbers confirming this and many other sources?

[Andrew Grieve]

Some good discussion here!

re: Putting constants within the @interface, there's a good SO article here:

https://stackoverflow.com/questions/35625247/android-is-it-ok-to-put-intdef-values-inside-interface

tldr - it would doing so would cause compiler errors for constants used by other android_library targets. Given this, I think it's probably a simple rule to just say that we should never nest them going forward.

The SO article also made me realize that our build system is not properly checking IntDefs across library boundaries, due to their SOURCE retention. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=870741 for this.

Even with the inability to put @IntDefs in maps (as donnd pointed out), and with our lint checks not fully working (which is fixable), I don't think there's much risk of code errors being introduced due to them vs. enums.

I've tried to capture this in a style guide update here (along with a few other edits):

https://chromium-review.googlesource.com/c/chromium/src/+/1162112

On Tue, Jul 17, 2018 at 6:48 PM, Marcin Wiacek <marcin...@mwiacek.com> wrote:

Hi Tommy,

 

Good and valuable question, thx – I have done my build without and with one patch and got the same numbers from build/android/method_count.py, details in the https://chromium-review.googlesource.com/c/chromium/src/+/1136879.

 

Cheers,

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/000201d41e20%2454ada580%24fe08f080%24%40mwiacek.com.

[Christian Dullweber]

Hi,

I just noticed a recent change that replaced a complex enum with a set of arrays that are indexed by an Intdef:https://chromium-review.googlesource.com/c/chromium/src/+/1215202/14/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java#b192

I agree that replacing simple enums that just represent integer constants with IntDefs is good if that reduces the binary size. I'm not sure if it is a good idea to replace more complex enums if this means that we have a number of arrays where any change has to preserve indexing and order. This seems to make it very easy to introduce bugs.
What do you think?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/java/CABiQX1U4f16yxQoa%2Bq9moGojZcN1Sj1L6BXrnvZ-%3D_x5HDUvXQ%40mail.gmail.com.

[David Turner]

On Thu, Sep 27, 2018 at 1:27 AM Christian Dullweber <dull...@chromium.org> wrote:

Hi,

I just noticed a recent change that replaced a complex enum with a set of arrays that are indexed by an Intdef:https://chromium-review.googlesource.com/c/chromium/src/+/1215202/14/chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java#b192

I agree that replacing simple enums that just represent integer constants with IntDefs is good if that reduces the binary size. I'm not sure if it is a good idea to replace more complex enums if this means that we have a number of arrays where any change has to preserve indexing and order. This seems to make it very easy to introduce bugs. 

What do you think?

There are already several cases in our sources where we need to ensure consistency between various arrays, which are sometimes even defined in different sources.

Usually, a comment along every declaration listing all the others to update and keep consistent are enough to avoid bugs during code reviews. E.g.:

Foo.java:

  // IMPORTANT: If you modify this list, also modify:

  // - FooWidgetList below.

  // - FooTitleText[] in FooTitle.java.
  static final String FooNames[] = { ... };

  // IMPORTANT: If you modify this list, also modify:

  // - FooNames above.

  // - FooTitleText[] in FooTitle.java.  static final Widget FooWidgetList[] = { ... };

FooTitle.java:

  // IMPORTANT: If you modify this list, also modify FooNames and FooWidgetList in Foo.java

  static String FooTitleText[] = { ... };

It is a little tedious to write at first, but it makes maintenance rather easy if these list do not change often (and if they do, you may have a different problem).

 

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKAfQi37MCb-Wi-Sr%3D5QpgOp91dPXrzns8Bbe3BH8M_12QYQAw%40mail.gmail.com.

[Bo Liu]

I'm reviewing a CL from Marcin that move @IntDef constants from being declared in a regular class to inside an @interface with RetentionPolicy = SOURCE. Andrew said above that that means these constants won't be accessible from other android_library targets. Is that problem solved already? If not, I think we should not be mass moving these to @interface, right?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CACnJMqokpZ3EAzC7W-G%3DVuMm53QhH7jTAfXh%2BsEwcRDgUeNrzw%40mail.gmail.com.

[Marcin Wiącek]

hmmm, Adrew said about compilation errors & we don't see them for months (we made mass move around Jul 2018 and currently I'm just moving single IntDef missed by accident or added in meantime).

[Bo Liu]

On Mon, Feb 4, 2019 at 2:58 PM Marcin Wiącek <mar...@mwiacek.com> wrote:

hmmm, Adrew said about compilation errors & we don't see them for months (we made mass move around Jul 2018 and currently I'm just moving single IntDef missed by accident or added in meantime).

It would only fail compile if it's used across android_library targets. Or maybe we had a solution for this already?

Either way, Andrew is the master of these things, so I'd let him chime in :)

[Marcin Wiącek]

Of course let's discuss all doubts and it would be perfect if master could give his opinion, from the second hand let's be precise - mass update was done long time ago and completed with full success, for now we're just opening old thread because of  CL about single IntDef (about 17 lines).

[Andrew Grieve]

The bug is here: https://bugs.chromium.org/p/chromium/issues/detail?id=870741

I don't think it's on anyone's radar for Q1.

I personally don't think it's very likely for a non-IntDef value to be passed to an IntDef parameter, so am not too worried about the check not working across targets.

On Mon, Feb 4, 2019 at 6:22 PM Marcin Wiącek <mar...@mwiacek.com> wrote:

... and I will want to summarize - even if some concrete very specific IntDef in very specific scenario make problem, can be always marked "don't convert" or "don't use source retention".

[Bo Liu]

Is it only the check that doesn't work? I interpreted your earlier comment as the constant values themselves inside @interface are not accessible to another android_library target.

If it's only the check, then yeah, I agree it's not a big deal.

[Andrew Grieve]

Yeah, I misunderstood the implications earlier. It should compile fine, and it's just the lint check that's broken.