Testlib: Add test that putIfAbsent replaces a null value #6217
Comments
|
Thanks Ben! Looks worth adding. If you or anyone else feels like making the PR we can merge it. If not, I hope we'd still get around to it sooner or later. |
|
@kevinb9n what would be the best way for me to get started contributing? This looks like a good first issue. |
|
Take a look at MapPutIfAbsentTester. These cases would be to allow null values so follow the pattern elsewhere for defining test matching constraints, e.g. It would also probably be good to review the default methods promoted from |
|
Hey ben, I made the code change but I'm running into some issues getting guava to build on my system. I get an error: package sun.misc does not exist any tips? |
|
I believe it should work, but I haven't run Guava's build for a long time. I know that Eclipse defaults to that error for restricted apis, if that is what you mean? If so, you can disable it under,
|
|
I gave it a build and another likely reason is if you are running on recent JDK which requires access modifiers. It worked for me on Java 11, and I believe the library targets Java 8. |
|
Take a look at my PR: #6225 |
|
Looks like a good first iteration. Of course that doesn't cover this scenario. I don't have commit access to the repository, so you'll need to have a champion on the Guava team marshall it through. |
|
I'm using IntelliJ. I'm having a hard time getting it to build so I can test that it all works. I signed the CLA. This will be my first contribution to a google project |
|
You can use command line, e.g. |
|
Thanks, I did that and now I get this message:
|
|
You are probably running on a newer JDK. It works fine for me on Java 11, but I get this on 17 due reflective access controls being restricted in later versions. |
…nd make builds work with newer JDKs. This should fix the error reported in #6217 (comment). I'm not sure if the Error Prone update was necessary for that or if only the `pom.xml` changes were. Still, it seems inevitable that we'll be forced to upgrade Error Prone eventually, and it's rarely a bad idea to update a plugin. This change is progress toward building and testing under Java 17 (#5801)... ...which we apparently regressed at when we enabled Error Prone (#2484). I've set this change up in a way that lets builds continue to work under JDK8 (which is potentially useful for #3990 or for anyone building Guava manually with an old JDK), albeit with Error Prone disabled. RELNOTES=n/a PiperOrigin-RevId: 484299394
|
Thanks for taking a look at this, and sorry for the build problems. I am making an attempt at fixing them in #6230. |
|
I think you can use Maven toolchains for handling this, but I am rusty with that build tool. |
This has some effects on us as we develop Guava, but it doesn't affect end users. Specifically: - When we build Guava with JDK8, we now have to disable Error Prone. (Again, users can still use Guava with JDK8, including with Error Prone. The effect is limited to people who are developing Guava.) - We can now successfully build Guava with Error Prone under more recent JDKs. That is, this change should fix the error reported in #6217 (comment). (I'm not sure if the Error Prone update was necessary for that or if only the other `pom.xml` changes were. Still, it seems inevitable that we'll be forced to upgrade Error Prone eventually, and it's rarely a bad idea to update a plugin.) This change is progress toward building and testing under Java 17 (#5801)... ...which we apparently regressed at when we enabled Error Prone (#2484). Oddly, it seems that part of our existing Error Prone setup is _required_ to continue building Guava under JDK8. (Such builds are potentially useful for #3990 or for anyone building Guava manually with an old JDK.) That's the case even though we're now disabling Error Prone for those builds. Again, all these changes affect only people who are developing Guava, not end users. RELNOTES=n/a PiperOrigin-RevId: 484299394
This has some effects on us as we develop Guava, but it doesn't affect end users. Specifically: - When we build Guava with JDK8, we now have to disable Error Prone. (Again, users can still use Guava with JDK8, including with Error Prone. The effect is limited to people who are developing Guava.) - We can now successfully build Guava with Error Prone under more recent JDKs. That is, this change should fix the error reported in #6217 (comment). (I'm not sure if the Error Prone update was necessary for that or if only the other `pom.xml` changes were. Still, it seems inevitable that we'll be forced to upgrade Error Prone eventually, and it's rarely a bad idea to update a plugin.) This change is progress toward building and testing under Java 17 (#5801)... ...which we apparently regressed at when we enabled Error Prone (#2484). Oddly, it seems that part of our existing Error Prone setup is _required_ to continue building Guava under JDK8. (Such builds are potentially useful for #3990 or for anyone building Guava manually with an old JDK.) That's the case even though we're now disabling Error Prone for those builds. Again, all these changes affect only people who are developing Guava, not end users. RELNOTES=n/a PiperOrigin-RevId: 485770768
|
@ben-manes does this also affect my lib? |
|
@Speiger yep, your |
|
It is also probably worth noting that |
|
Ty @ben-manes Yeah I allow null values in my ConcurrentMaps, since they are guava styled its actually rather easy to support null. |
|
The segment style in Guava's is forked from Java 5's ConcurrentHashMap. There are definite gotchas with atomicity and null values, and I think allowing null values at all was a mistake for Map. It is best replaced by a sentinel value, e.g. Optional, which has the same cpu and memory footprint (as no one actually makes a valueless entry object for this case in their maps). My general rule is to follow Doug Lea's decisions, as if he found a feature too hard and confusing to support then everyone else will get it wrong (either the implementor or user, as too much of a footgun for a consistent understanding). |
|
@ben-manes while i agree to 99% of it, the one thing I disagree, I just realised so that's why the comment comes just now. |
This was shown in a recent Java Collections Puzzlers. The JavaDoc for
Map.putIfAbsentstates the following.I found that multiple custom maps which permit null values do not honor this peculiarity (and similar for
computeIfAbsent). For example fastutils' adopted the testlib andObject2ObjectOpenHashMappasses its suite, but it mistakenly does not replace the value as shown above.The text was updated successfully, but these errors were encountered: