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

[Atsushi Eno]

Hi Marek,

Please, don't remove devel-list from CC.

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

> 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);

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

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

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

Thanks,
Atsushi Eno