Conversation
wmdietl
left a comment
wmdietl
left a comment
There was a problem hiding this comment.
@aosen-xiong @thisisalexandercook We had discussed this PR in the past. Let's go through this and related PRs next week and decide which direction to go.
| * @param annotationScope the element that the conservative default might apply to | ||
| * @return whether the conservative default applies to the given element | ||
| */ | ||
| public boolean applyOptimisticDefaults(Element annotationScope) { |
There was a problem hiding this comment.
Most of this code is just copy-and-paste of applyConservativeDefaults. Can you find a way to share more of the common logic?
| if (applyConservativeDefaults(annotationScope)) { | ||
| for (Default def : uncheckedCodeDefaults) { | ||
| for (Default def : conservativeUncheckedCodeDefaults) { | ||
| if (!typeVarUseDef || def.location != TypeUseLocation.TYPE_VARIABLE_USE) { |
There was a problem hiding this comment.
This also duplicates the logic in the else branch.
Can you first set which defaults to use and then iterate over that set once?
| } | ||
| } | ||
|
|
||
| for (TypeUseLocation loc : CONSERVATIVE_UNCHECKED_DEFAULTS_TOP) { |
There was a problem hiding this comment.
Why iterate over all these sets? Won't most of these conflict with defaults that were already set above?
I think this logic needs some cleaning up.
| */ | ||
| protected void addUncheckedStandardDefaults(QualifierDefaults defs) { | ||
| defs.addUncheckedStandardDefaults(); | ||
| protected void addUncheckedDefaults(QualifierDefaults defs) { |
There was a problem hiding this comment.
Why rename this method? It will still set the standard defaults. What changed?
| * @param kindOfCode source or bytecode | ||
| * @return whether optimistic defaults should be used | ||
| */ | ||
| public boolean useOptimisticDefault(String kindOfCode) { |
| "checkEnclosingExpr", | ||
|
|
||
| // Whether to use optimistic defaults for bytecode and/or source code. | ||
| // The option takes same arguments as "useConservativeDefaultsForUncheckedCode". |
There was a problem hiding this comment.
The ordering of the options should be switched, instead of having a forward reference here.
|
|
||
| The new command-line option `-AuseOptimisticDefaultsForUncheckedCode` takes `source` and `bytecode` argument, similar to | ||
| `-AuseConservativeDefaultsForUnCheckedCode` but apply to optimistic default, that is, Top for method parameter type and | ||
| Bottom for method return and field type. |
There was a problem hiding this comment.
We also need to go through the manual and discuss this new option there.
2 participants
Fixes #1359.
I disable the JSpecify reference checker CI because the method name are changing. I will enable and make the change at there (if necessary) and enable the CI.