[Mono-devel-list] Compiler warnings: what to report (Re: System.XML warning)

[Atsushi Eno]

Hello,

Marek Safar wrote:
> Hello,
> 
>>>>>
>>>>> System.Xml/XmlReader.cs(1051) warning CS0169: The private method 
>>>>> 'System.Xml.XmlReader.CheckSupport()' is never used
>>>>
>>>> This is used in NET_2_0 part of code.
>>>> Should we enclose ALL of such members in _nasty_ preprocessor
>>>> directives? Does it ease developers read it?
>>>
>>> Yes, it is easier to read and easier to maintain. If you don't 
>>> enclose your members in #if NET_2_0 directive how can I know
>>> that a field is not valid for NET_1_x ?. I have to search in your 
>>> code and try to find out whether the field is valid or not.
>>
>> IMO having ifdefs are much more nasty than just having unused fields.
>> If there are ifdefs in effect, in every block (not only field 
>> declarations) you must grep up to find whether it is valid block
>> or not.
> 
> I respect your opinion but I really don't like searching in not my code 
> and looking in which .NET version is this variable available.
> BTW, You can use partial classes for really ugly constructs.

Hmm, then I think we have fifty-fifty game here. I don't think I
can convince you and vice versa.

It also means other points of view are welcome.

>> If those warning output REALLY annoys developers, I'd just add
>> /nowarn:169 as well as 162, 612 and 618.
> 
> I am not sure whether every developer is annoyed by these warnings.

I wonder what is the actual need to fix warnings then.

>> Well, aside this code, there is possibility that those extra fields
>> might be used from other assemblies. For example, I used to have
>> XQueryCommandImpl type and field in System.Xml.dll and referenced it
>> from System.Data.SqlXml.dll via reflection.
> 
> OK, then please declare this field/method as internal. It is easier to 
> see that the field is hacked from outside.

I can easily do that, but I don't understand why we have to change
a member's access modifier just for avoiding warnings (I don't think
the difference between private and internal is for such intent).

>> Please don't send me but post it to the mail list so that everyone
>> can have a chance to try.
> 
> Only System.XML and System.Web have problem is so many warnings. The 
> rest of
> assemblies is OK. So, I addressed only you and Gonzalo.

I think the reason why we have such a lot of warnings only in those
assemblies is because we have large part of NET_2_0 implementation
(rather than just a stub) mainly in sys.xml and sys.web. That's part
of why I think your patch matters for otheres, not only for Gonzalo,
Lluis and me.

>>> But csc is really not good in this area. They made some improvement 
>>> for 2.0. But even
>>> simple cases are still not detected.
>>
>> Yes, csc 2.0 seems added some additional code analysis. But I doubt
>> if the remaining check is "better". It sounds like design principle
>> difference.
> 
> I think it is bug, consider this
> 
> class X {
>    int x;
> 
>    public X (int x)
>    {
>        this.x = x;
>    }
> }
> 
> Any of csc 1.x, 2.0 versions don't report warning for this code.
> But mcs reports: warning CS0169: The private field 'X.x' is never used
> And I think it is 100% correct.

Well, for this case it looks fine (maybe it should be like "X.x
is assigned but never used" though).

Atsushi Eno