[RFR]: Field suppression list

[Jean Christophe Beyler]

Hi Arthur,

FWIW, the comment here:

>         - I would really find a '#' and replace it with '\0' if found
>
> If we have to check for '#' either way, I think bailing out when we see it
is fine.

I don't entirely agree. User input is always error-prone and I could easily
do:

# Ignore this comment
 # But ignore this one
whatever #Incomplete line but now accepted by the system since it does a
"%s %s"
whatever really #But this also is accepted because sscanf does not look
afterwards...

In theory, we'd like the system to ignore the three first lines at least
since they are not correct, your code will accept the second and third...

If you don't want to fix it now that's ok but it should be something fixed
a bit to be a little bit more robust, no?
Jc


On Tue, Jul 9, 2019 at 3:23 PM Arthur Eubanks <aeubanks at google.com> wrote:

> On Tue, Jul 9, 2019 at 9:06 AM Jean Christophe Beyler <jcbeyler at google.com>
> wrote:
>
>> Hi Arthur,
>>
>> Sorry for the late reply :)
>>
>>
>> http://cr.openjdk.java.net/~aeubanks/tsanfieldsupp/webrev.00/src/hotspot/share/classfile/tsanIgnoreList.hpp.html
>>
>>   33 // Lines that start with '#' are considered comment.
>>
>> -> are considered comments.
>>
>> Done
>
>>
>>   36 // Here are few examples.
>>
>> -> Here are a few examples:
>>
>> Done
>
>>
>> http://cr.openjdk.java.net/~aeubanks/tsanfieldsupp/webrev.00/src/hotspot/share/classfile/tsanIgnoreList.cpp.html
>>
>>   - line 156 could be removed (extra empty line)
>>
>> Done
>
>>   - whereas line 175 could use an extra line :)
>>
>> Done
>
>>   - Is there a reason that we are using fgetc? I would just use fgets and be done with all this logic handling lines...
>>
>> Done
>
>>   - FWIW, I think you should extract the whole file parsing into a parse_from_file method, which would make the init : open a file, parse it via parse_from_file, close it
>>
>> Done
>
>>   - parse_from_line:
>>
>>      - I find this error prone:
>>
>>         - I would really find a '#' and replace it with '\0' if found
>>
>> If we have to check for '#' either way, I think bailing out when we see
> it is fine.
>
>>         - then I would do a sscanf and check its return
>>
>>  Done.
>
>>         - I would add a few extra lines to better understand this code btw :)
>>
>> Added some comments
>
>>
>>   -   35     Exact,
>>
>>      -> If we are going to do = 1, = 2, ... ; might as well do = 0
>>
>> Done
>
>>
>>   41   FieldMatcher(const Symbol* class_name, const Symbol* field_name,
>>   42                Mode class_mode, Mode field_mode, FieldMatcher* next)
>>
>>      -> Somehow class_name, class_mode, field_name, field_mode, next seems like a better order when looking at it (to me :-))
>>
>> Done
>
>>
>> Thanks,
>>
>> Jc
>>
>> Also added some tests for testing Any and Prefix matching
> Updated webrev:
> http://cr.openjdk.java.net/~aeubanks/tsanfieldsupp/webrev.01/
>
>


-- 

Thanks,
Jc