[Mono-devel-list] Patch for full-featured mcs /doc support

[Marek Safar]

Hello Eno,

>Finally I finished the complete patch set for /doc features,
>tests and errors, including warning checks and result document
>comparison as long as it makes sense ... except for one thing
>I couldn't complete; cref attribute handling which has no
>information how it is checked.
>
>Would you please review this new patch? Am very sorry but I
>significantly changed the code (like adding much changes on
>parser/tokenizer to detech invalid comments, removing XmlElement
>field from member, using XmlWriter directly to output, etc).
>
>  
>
Very, very good progress, tests are wonderful. I think that Miguel
should prepare official stamp ;-)

My comments:

+ public string consume_doc_comment ()
+ {
+ if (xml_comment_buffer.Length > 0) {
+ string ret = null;
+ if (xmlCommentSavePoint > 0) {
+ ret = xml_comment_buffer.ToString (0, xmlCommentSavePoint);
+ xml_comment_buffer.Remove (0, xmlCommentSavePoint);
+ } else {
+ ret = xml_comment_buffer.ToString ();
+ xml_comment_buffer.Length = 0;
+ }
+ xmlCommentSavePoint = 0;
+ return ret;
+ }
+ else
+ return null;
+ }

The last else is useless and xmlCommentSavePoint = 0 should be only for
xmlCommentSavePoint > 0.

+ public enum XmlCommentState {
+ OK,
+ NG,
+ Error
+ }

What is NG ?

+ MemberInfo mi = FindDocumentedMember (type, memberName,
parameterTypes, ds, out warnResult);
+ switch (warnResult) {
+ case 1581:
+ Report.Warning (1581, 1, Location, "Invalid return type in XML comment
cref attribute '{0}'", cref);
+ return;
+ case 1584:
+ Report.Warning (1020, 1, Location, "Overloadable {0} operator is
expected", parameterTypes.Length == 2 ? "binary" : "unary");
+ Report.Warning (1584, 1, Location, "XML comment on '{0}' has
syntactically incorrect attribute '{1}'", GetSignatureForError (), cref);
+ return;
+ }

Why don't do it inside FindDocumentMember to avoid copy&paste.


- : IDENTIFIER
+ : IDENTIFIER
+ {
+ tmpComment = Lexer.consume_doc_comment ();
+ Lexer.doc_state = XmlCommentState.OK;
+ }
OPEN_PARENS opt_formal_parameter_list CLOSE_PARENS
{
oob_stack.Push (lexer.Location);

- current_local_parameters = (Parameters) $3;
+ current_local_parameters = (Parameters) $4;
}
opt_constructor_initializer
{
Location l = (Location) oob_stack.Pop ();
- $$ = new Constructor (current_class, (string) $1, 0, (Parameters) $3,
- (ConstructorInitializer) $6, l);
+ $$ = new Constructor (current_class, (string) $1, 0, (Parameters) $4,
+ (ConstructorInitializer) $7, l);
}

I am confuse, what has been changed.

+ } catch (NotImplementedException ex) {
+ Report.Error (1569, "Error generating XML documentation file '{0}'
('{1}')", xml_documentation_file, ex.Message);
+ return false;

Does make a sense to catch this exception ?

+ vd.DocComment = ConsumeStoredComment ();
+ Lexer.doc_state = XmlCommentState.OK;

Why don't call this tricky property in ConsumeStroredComment I found
only one place where this combination is missing (bug ??). Should be
possible also to rename this property ?
BTW: I think we should call this call only for /doc option.

+ private MemberInfo FindDocumentedMember (Type type, string memberName,
Type [] paramList, DeclSpace ds, out int warningType)

This is heavyweight. I think we can optimize or reuse some of the
existing code here. But we will have to do it sometimes later.


To Miguel, Martin: Why we have TypeContainer instead of DeclSpace in
MemberCore class. I faced off with this many many times and here it is
again. It looks like I will have to do big clean up ;-)


Marek

>------------------------------------------------------------------------
>
>Index: Makefile
>===================================================================
>--- Makefile	(revision 36457)
>+++ Makefile	(working copy)
>@@ -13,7 +13,7 @@
> # force this, we don't case if CSC is broken. This also
> # means we can use --options, yay.
> 
>-MCS = MONO_PATH="$(topdir)/class/lib/$(PROFILE)$(PLATFORM_PATH_SEPARATOR)$$MONO_PATH" $(INTERNAL_MCS)
>+MCS = MONO_PATH="$(topdir)/class/lib/$(PROFILE) $(PLATFORM_PATH_SEPARATOR) $$MONO_PATH" $(INTERNAL_MCS)
> endif
> 
> # We don't want debugging info :-)
>@@ -26,7 +26,8 @@
> # He may also move some to TEST_EXCLUDE_net_2_0 if some of the merges are inappropriate for GMCS.
> #
> NEW_TEST_SOURCES_common = test-294 test-304 test-305 test-306 test-307 test-318 mtest-5-dll mtest-5-exe \
>-			test-319-dll test-319-exe
>+			test-319-dll test-319-exe \
>+			$(TEST_SOURCES_XML)
> 
> #
> # Please do _not_ add any tests here - all new tests should go into NEW_TEST_SOURCES_common
>@@ -110,14 +111,14 @@
> 	$(TEST_SOURCES_common) $(TEST_SOURCES_$(PROFILE)) $(TEST_SOURCES_$(PLATFORM)))
> 
> # These tests load User32.dll and/or Kernel32.dll
>-TEST_SOURCES_win32 = test-50 test-67
>+TEST_SOURCES_win32 = test-67
> 
> ## FIXME: Need to audit.  Maybe move to 'TEST_EXCLUDES_linux' and 'TEST_EXCLUDES_win32' as approprate
> # A test is a 'no pass' if it fails on either windows or linux
> # Test 120 does not pass because the MS.NET runtime is buggy.
> 
> TEST_NOPASS = test-120 test-132 test-133 a-parameter4.cs 
>-#	test-28 test-45 test-53 test-91 test-102 test-106 test-107 test-122 test-66 test-177
>+#	test-28 test-45 test-50 test-53 test-91 test-102 test-106 test-107 test-122 test-66 test-177
> 
> # The test harness supports running the testcases in parallel.  However, we still need to
> # provide test-ordering rules to support multi-file testcases.  By default, any test named
>@@ -155,7 +156,7 @@
> 
> test-local: 
> 
>-run-test-local: multi test-harness test-casts
>+run-test-local: multi test-harness test-casts xmldocdiff.exe xml-doc
> 
> test-everything:
> 	$(MAKE) PROFILE=default run-test
>@@ -222,3 +223,30 @@
> 	$(INTERNAL_ILASM) /dll property-il.il
> 	$(CSCOMPILE) /r:property-il.dll property-main.cs /out:property-main.exe
> 	$(TEST_RUNTIME) property-main.exe
>+
>+
>+#
>+# Test for /doc option; need to compare result documentation files.
>+#
>+
>+TEST_SOURCES_XML = \
>+	xml-001 xml-002 xml-003 xml-004 xml-005 xml-006 xml-007 xml-008 xml-009 xml-010 \
>+	xml-011 xml-012 xml-013 xml-014 xml-015 xml-016 xml-017 xml-018 xml-019 xml-020 \
>+	xml-021 xml-022 xml-023 xml-024 xml-025 xml-026
>+
>+# currently no formalization on 'cref' attribute was found, so there are some
>+# differences between MS.NET and mono.
>+TEST_SOURCES_XML_PENDING = xml-027
>+
>+xmldocdiff.exe:
>+	$(CSCOMPILE) xmldocdiff.cs
>+
>+xml-doc:
>+	@$(MAKE) -s xml-doc-run
>+
>+xml-doc-tests := $(filter xml-%, $(TEST_SOURCES))
>+
>+xml-doc-run:
>+	@echo "Testing /doc outputs..."
>+	@test -z "$(xml-doc-tests)" || for i in ''$(xml-doc-tests); do $(TEST_RUNTIME) xmldocdiff.exe $$i-ref.xml dir-$(TEST_TAG)/$$i.xml; done
>+
>  
>