8210496: Improve filtering for classes with security sensitive fields

[Alan Bateman]

On 14/09/2018 20:16, Peter Levart wrote:
> Hi Alan,
>
> Just a few comments about implementation.
>
> In Reflection:
>
>  292             boolean shouldSkip = false;
>  293             for (String filteredName : filteredNames) {
>  294                 if (member.getName() == filteredName) {
>  295                     shouldSkip = true;
>  296                     break;
>  297                 }
>  298             }
>
> ...now that filteredNames is a Set<String>, you could simplify this 
> loop to:
>
>         shouldSkip = filteredNames.contains(member.getName());
I was trying to keep the changes to the filtering implementation as 
minimal as possible but you are right that replace the array with a set 
means the loop can be replaced (thanks!).

>
> :
>
> I don't know if it's better, but you could use just the immutable Map 
> without HashMap. Instead of:
>
>  262         map = new HashMap<>(map);
>  263         map.put(containingClass, Set.copyOf(names));
>  264         return map;
>
> you could do this:
>
>         @SuppressWarnings("unchecked")
>         Map.Entry<Class<?>, Set<String>>[] entries = 
> map.entrySet().toArray(new Map.Entry[map.size() + 1]);
>         entries[entries.length - 1] = Map.entry(containingClass, 
> Set.copyOf(names));
>         return Map.ofEntries(entries);
>
> This should work because the registerFilter method is called at most 
> once per map/containingClass.
The existing code is easy to read. Using an immutable map has a few 
advantages but using map entries is easy to get wrong. I think I'll 
leave it as is for now.  There are a few additional classes that should 
also be added to the filter in another round so that might be the time 
to re-visit it.

I've put a new webrev here, the only change is the replacement of the 
old loop to use contains as you suggested:
   http://cr.openjdk.java.net/~alanb/8210496/2/webrev/index.html

-Alan