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

[Marek Safar]

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.

>
> 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.

>
>>>> System.Xml/XmlTextReader.cs(1672) warning CS0168: The variable 
>>>> 'dummyValue' is declared but never used
>>>
>>>
>>>
>>>
>>> It is _required_ even though it is actually not used (otherwise mcs/csc
>>> will reject this code). CSC never reports it as warning and I think
>>> that regarding it as a warning is bad design.
>>>
>> Is it some managed <-> unmanaged work ?
>
>
> If you tried to remove that assignment statement for the dummy value,
> csc will complain that:
>
>     System.Xml\XmlTextReader.cs(1674,23): error CS0201: Only
>     assignment, call, increment, decrement, and new object
>     expressions can be used as a statement
>
> and mcs saying that:
>
>     syntax error, got token `SEMICOLON'
>     System.Xml\XmlTextReader.cs(1674) error CS1002: Expecting `;'

I will have a look what is going here.

>
> 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.

>
>>>> System.Xml.Xsl/XslTransform.cs(604) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xsltCleanupGlobals()' 
>>>> is never used
>>>> System.Xml.Xsl/XslTransform.cs(611) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlNewDoc( string)' is 
>>>> never used
>>>> System.Xml.Xsl/XslTransform.cs(614) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlSaveFile( string,  
>>>> System.IntPtr)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(626) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlCleanupParser()' is 
>>>> never used
>>>> System.Xml.Xsl/XslTransform.cs(629) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlDocDumpMemory( 
>>>> System.IntPtr, ref System.IntPtr&, ref int&)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(632) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlFree( 
>>>> System.IntPtr)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(649) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.valuePop( 
>>>> System.IntPtr)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(652) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.valuePush( 
>>>> System.IntPtr,  System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' 
>>>> is never used
>>>> System.Xml.Xsl/XslTransform.cs(655) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathFreeObject( 
>>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(658) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewCString( 
>>>> string)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(661) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewFloat( 
>>>> double)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(664) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewBoolean( 
>>>> int)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(667) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathNewNodeSet( 
>>>> System.IntPtr)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(670) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathCastToBoolean( 
>>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(673) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathCastToNumber( 
>>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(676) warning CS0169: The private 
>>>> method 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathCastToString( 
>>>> System.Xml.Xsl.UnmanagedXslTransform.xpathobject*)' is never used
>>>> System.Xml.Xsl/XslTransform.cs(679) warning CS0169: The private 
>>>> method 
>>>> 'System.Xml.Xsl.UnmanagedXslTransform.xmlXPathStringFunction( 
>>>> System.IntPtr,  int)' is never used
>>>
>>>
>>> Why are they reported as never used? They are actually in use.
>>
>>
>> Are you sure that they are used ?
>>
>> I tried to remove 
>> 'System.Xml.Xsl.UnmanagedXslTransform.xsltCleanupGlobals' and no 
>> compilation error.
>
>
> Mmm, sorry then I should have checked more in details. At least
>
> >>> System.Xml.Xsl/XslTransform.cs(649) warning CS0169: The private
> >>> method 'System.Xml.Xsl.UnmanagedXslTransform.valuePop(
> >>> System.IntPtr)' is never used
>
> this is really in use.

Thanks, it looks like bug in my patch.

>
> I'll check for each unmanaged methods and comment out if they are
> unused.
>
>>> BTW how can we verify that those sources became clean after
>>> manual warning elimination?
>>
>>
>> I will send you mcs patch.
>
>
> 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.

>
>>> Also, I think that some part of those warning report feature are
>>> problematic. For example,
>>>
>>>     - I think there is no (or little) need to report not-in-use
>>>       private methods. When we found such methods that we don't
>>>       know what it is, we usually grep (usually) single file
>>>       and notice that it is not in use. I believe that why MS
>>>       csc has such reporting feature for unused fields is that
>>>       they might be confused with local variables or parameters.
>>>       I don't think mcs should not let developers to be puritan
>>>       that believes ALL unused members MUST be eliminated.
>>
>>
>>  From my point of view, there are several reasons why report "unused" 
>> code.
>> - I like every feature which tells me that something is "wrong" with 
>> my code.
>> - Your code can be easily faster/smaller. When you remove class 
>> member you allocate less.
>> When you remove method sometimes it leads to next methods/fields 
>> clean up.
>>
>> Maybe I am wrong but why should I keep unused code ?
>
>
> From my point of view, there are many reasons not to report "unused" 
> code.
> - I like every feature which tells me that something is "wrong" with my
>   code, only when it is REALLY wrong.
> - It is too trivial contribution to performance to remove unused code,
>   unless there are heavy reflection work.
> - Those puritan warnings really discourages us at every point of
>   incomplete development where we wonder if our refactory is fine.
>   For example, suppose we were developing UnmanagedXslTransform and
>   going to support all exposed functionality in unmanaged library,
>   Checking every import is just a pain.

No, I don't intercept you to have warnings in your code when is under 
development.
I only think that 50 same warnings in one assembly is simply too many.

>
>>>
>>>     - As I noted above, there are unexpected unused field check
>>>       that at least csc does not regard as should-be-warned.
>>>
>> If you know about any wrong warnings please let me know.
>
>
> As for that unused field, I think I explaind understandably, though
> I don't know that is the merginal line between those code that is
> worthy of being warned and those code that is not.
>
>> 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.

Marek