[Mono-devel-list] Re: Improved and fixed mcs /doc patch

[Marek Safar]

Hello Eno,

>Please, don't remove devel-list from CC.
>
>  
>
As you want ;-)

>>I love these tests. Just add comparison to makefile and it will be great.
>>    
>>
>
>Actually we can't. They are literally different. But we can still add
>comparison code by using canonicalization or something else.
>
>  
>
I don't understand where is the problem, with line ending or somewhere else.

>>It would be really good to have this CreateMemberComment inside of class derived from MemberCore instead of in the lexer.
>>e.g
>>
>>method.SetComment (xml_data);
>>    
>>
No, original idea was little different. Look at this.

+ if (Lexer.xml_comment_buffer.Length > 0)
+ e.Documentation = CreateMemberComment ("E:" + MakeName (e.MemberName));

It seems to me that you are doing things that should be done inside a
class outside. When I have operation that works just with class members
why it is outside of class.

>I could not find strong reason to do so and have separate trivial
>overrides in each derived MemberCore just for name string (to have
>"M:", "P:", "F:" ...). What I would do is
>"method.SetComment (string name, string xmlcomment)" and hopefully
>
>  
>
>>+			string file = el.GetAttribute ("file");
>>+			string path = el.GetAttribute ("path");
>>+			if (file == "")
>>+				Report.Warning (1590, 1, Location, "Invalid XML 'include' element; Missing 'file' attribute.");
>>+			else if (path == "")
>>+				Report.Warning (1590, 1, Location, "Invalid XML 'include' element; Missing 'path' attribute.");
>>Both file and path could not be "".
>>    
>>
>
>csc does not report both, even if both of them are missing.
>
>  
>
>>+		internal void CheckDocumentation (DeclSpace ds)
>>+		{
>>+			if (Documentation != null) {
>>+				foreach (XmlElement el in Documentation.SelectNodes (".//include"))
>>+					HandleInclude (el);
>>+				Documentation = null; // release the reference
>>+			}
>>+			else if (RootContext.NeedDocument &&
>>+				IsExposedFromAssembly (ds) &&
>>+				// There are no warnings when the container also
>>+				// misses documentations.
>>+				(ds == null || ds.Documentation != null)) {
>>+				string name = MemberName.GetFullName ();
>>+				if (ds != null)
>>+					name = String.Concat (ds.MemberName.GetTypeName (), ".", name);
>>+				Report.Warning (1591, 4, Location,
>>+					"Missing XML comment for publicly visible type or member '{0}'", name);
>>+			}
>>+		}
>>
>> * Better should be move whole method to MemberCore.FixupDocumentaion
>>    
>>
>
>It was used in Operator. It was historical, and now merged.
>
>  
>
>>+			else if (RootContext.NeedDocument &&
>>+				IsExposedFromAssembly (ds) &&
>>
>> * RootContext.NeedDocument is always true, isn't it ?
>>    
>>
>
>No. It is true only when /doc is passed to the command line
>(otherwise that error should not be put).
>  
>

OK, but I thought that public virtual void FixupDocument (DeclSpace ds)
is called only when RootContext.NeedDocument is true.;

>  
>
>>+				string name = MemberName.GetFullName ();
>>+				if (ds != null)
>>+					name = String.Concat (ds.MemberName.GetTypeName (), ".", name);
>>+				Report.Warning (1591, 4, Location,
>>+					"Missing XML comment for publicly visible type or member '{0}'", name);
>>
>> * Report.Warning (1591, 4, Location, "Missing XML comment for publicly visible type or member '{0}'", GetSignatureForError ());
>>  can be better
>>    
>>
>
>Ok, fixed.
>
>  
>
>>Did you run error tests (mcs/error) too ?.
>>    
>>
>
>mhm, didn't notice. Maybe having separate tests for warnings and
>having option warnaserror would be nicer.
>  
>

No, I want to be sure that you didn't break any of error tests.

>Code (with reorganizing tests) will follow, maybe tomorrow.
>
>
>  
>
OK

Thanks,
Marek