Centralize comment parsing #3030

InsertCreativityHere · 2024-10-31T17:26:47Z

This PR removes a bunch of duplicate code we have for parsing comments. Most from slice2matlab and slice2swift.
Originally I was just going to fix this in Swift (see #2631), but I just went ahead and ripped it all out at once.
While I was at it, I also accidentally found/fixed some small bugs in our doc comment generation along the way.

Currently we have code for parsing Slice doc comments scattered around the various compilers, much of it nearly identical.
Sometimes we even invent a new type for holding the comment data DocElement; a near duplicate of Comment.

The main reason for this duplication was that every language had it's own formatting for doc-links.
My 'fix' for this is that now you can pass in an std::function to parseComment which is used to handle language-specific link formatting.

InsertCreativityHere · 2024-10-31T17:29:36Z

cpp/src/slice2cpp/Gen.cpp

+    /// Returns a doxygen formatted link to the provided Slice identifier.
+    string cppLinkFormatter(string identifier)
+    {
+        return "{@link " + fixKwd(identifier) + "}";


Apparently we weren't escaping names in C++ links...
Linking to an identifier that is a C++ keyword is a very niche case, but still good to fix.

InsertCreativityHere · 2024-10-31T17:30:04Z

cpp/src/slice2java/Gen.cpp

+    /// Returns a javadoc formatted link to the provided Slice identifier.
+    string javaLinkFormatter(string identifier)
+    {
+        return "{@link " + Slice::JavaGenerator::fixKwd(identifier) + "}";


Similarly in Java, we forgot to escape our linked-to identifiers...

InsertCreativityHere · 2024-10-31T17:30:15Z

cpp/src/slice2java/Gen.cpp

@@ -1753,28 +1761,6 @@ Slice::JavaVisitor::writeDataMemberInitializers(Output& out, const DataMemberLis
    }
 }

-StringList


Completely dead code.

InsertCreativityHere · 2024-10-31T17:31:07Z

cpp/src/slice2java/JavaUtil.cpp

@@ -780,7 +780,7 @@ Slice::JavaGenerator::output() const
 // otherwise, return the name unchanged.
 //
 string
-Slice::JavaGenerator::fixKwd(const string& name) const


This function was switched from a const instance function to a static function.
There was no reason for it to not be static.

InsertCreativityHere · 2024-10-31T17:34:03Z

cpp/src/slice2js/Gen.cpp

@@ -510,47 +489,43 @@ Slice::JsVisitor::writeConstantValue(
    return os.str();
 }

-StringList
-Slice::JsVisitor::splitComment(const ContainedPtr& p)


After refactoring, this function was only used by writeDocCommentFor (the function underneath it). So I just inlined it into that function.

InsertCreativityHere · 2024-10-31T17:39:00Z

cpp/src/slice2swift/SwiftUtil.cpp

+    CommentPtr doc = p->parseComment(swiftLinkFormatter, true, true);
+    if (!doc)
+    {
+        return;
+    }


The only difference is that the parseComment in the parser can return nullptr if there is no comment, whereas the old slice2swift specific parseComment would always return a DocElements.

So, now we do need to check if nullptr was returned before generating anything.

InsertCreativityHere · 2024-10-31T17:41:26Z

cpp/src/slice2swift/SwiftUtil.cpp

+    // Skip if there are no doc comments.
+    StringList docOverview = doc->overview();
+    StringList docMisc = doc->misc();
+    StringList docSeeAlso = doc->seeAlso(); // TODO we check seeAlso, but don't write any of in the generated comment?


There are definitely still bugs in the slice2swift doc generation, I TODOed the ones I found, and plan to fix them when I fix #2274.

InsertCreativityHere · 2024-10-31T17:42:47Z

cpp/src/Slice/Parser.h

@@ -375,8 +375,10 @@ namespace Slice
        int line() const;

        std::string comment() const;
-        CommentPtr parseComment(bool stripMarkup) const;
-        CommentPtr parseComment(const std::string& text, bool stripMarkup) const;


We only need one function, the 2nd one was a hack-around to let ice2slice massage it's links before parsing. The new linkFormatter approach solves this problem though.

InsertCreativityHere · 2024-10-31T17:44:10Z

cpp/src/Slice/Parser.cpp

@@ -853,6 +848,7 @@ Slice::Contained::parseComment(const string& text, bool stripMarkup) const
        {
            if (!line.empty())
            {
+                state = StateMisc; // Reset the state; `@see` cannot span multiple lines.


Fixes a bug where we'd sometimes generate multi-line see statements.
These should always be a single line like: @see MyType.op79

InsertCreativityHere · 2024-10-31T17:45:35Z

cpp/src/Slice/Parser.cpp

+        // Fix any javadoc using the provided link formatter.
+        const string link = "{@link ";


There should always be a space after the @link.
This code would let through things like {@linkStringSeq} which is bogus.

…er...

InsertCreativityHere · 2024-10-31T17:56:22Z

cpp/src/slice2cpp/Gen.cpp

+    /// Returns a doxygen formatted link to the provided Slice identifier.
+    string cppLinkFormatter(string identifier) { return "{@link " + fixKwd(identifier) + "}"; }
+


Apparently we weren't escaping names in C++ links...
Linking to an identifier that is a C++ keyword is a very niche case, but still good to fix.

bernardnormier

It looks good except for the handling of deprecated, which should as per #2970:

a) in the Slice parser, the deprecated metadata no longer inserts "deprecated" in the associated doc-comment, if any. This insertion was incorrect.

b) in the Slice compilers, we no longer generate a language-specific @deprecated doc-comment for servants and servant operations.

#2970 was not comprehensive, and this was by accident.

bernardnormier · 2024-10-31T18:27:07Z

cpp/src/Slice/Parser.cpp

-Slice::Contained::parseComment(const string& text, bool stripMarkup) const
-{
-    if (text.empty())
+    // Check for deprecated metadata and add it to the comment if necessary.


Please don't do that. I removed this behavior in #2970, and looking at this PR, it appears I forgot do removed it from Swift and MATLAB. Please fixes these too.

In fairness, the comment code for Swift and MATLAB was completely separate from all the other comment code, so it's not surprising.

I removed all the includeDeprecated stuff, and am happy to see it go!

bernardnormier · 2024-10-31T18:32:47Z

cpp/src/Slice/Parser.h

-        CommentPtr parseComment(const std::string& text, bool stripMarkup) const;
+        CommentPtr parseComment(
+            std::function<std::string(std::string)> linkFormatter,
+            bool includeDeprecatedMetadata = false,


See comment in .cpp

bernardnormier · 2024-10-31T18:33:44Z

cpp/src/Slice/Parser.cpp

-            // Strip HTML markup and javadoc links.
-            string::size_type pos = 0;
+            // Strip HTML markup.
+            pos = 0;


move to line 644

bernardnormier · 2024-10-31T18:38:14Z

cpp/src/slice2js/Gen.cpp

-StringList
-Slice::JsVisitor::splitComment(const ContainedPtr& p)
+void
+Slice::JsVisitor::writeDocCommentFor(const ContainedPtr& p, bool includeDeprecated)


This includeDeprecated parameter doesn't sound correct to me.

The generation of deprecated doc-comment tags should be totally independent of whether the Contained is deprecated.

I didn't look too closely at this function, but I think this field might still be useful.

To my knowledge, javascript doesn't have a standalone attribute like [Obsolete] or [[deprecated]].
The only way to mark something deprecated is at the doc-comment level, so maybe linters and other tools will check for this doc comment tag?

And there are some things that we don't want to mark as deprecated, like the generated service skeletons.

So you want to add a special JS-only logic like: Container is deprecated but there is no @deprecated doc-comment => generate @deprecated JS doc-comment?

Sounds ok, but definitely deserves a comment.

Then:

And there are some things that we don't want to mark as deprecated, like the generated service skeletons.

Yes, I actually did something similar for C++:

ice/cpp/src/slice2cpp/Gen.cpp

Line 370 in e5261ff

writeDocSummary(Output& out, const ContainedPtr& p, GenerateDeprecated generateDeprecated = GenerateDeprecated::Yes)

So you want to add a special JS-only logic like

No, the inverse of that. I want to add exactly what you described for C++!
I was just justifying why we might want that behavior, because I was unaware it already existed :)

I'll co-opt your code into the parser, and then use it for all the different languages.
But since this PR is already pretty big, I'd prefer to leave that for when I refactor all the deprecated stuff,
since there's like 3 separate issues for cleaning up how deprecated works...

Say you define in Slice:

/// The base interface for all Widgets ["deprecated"] interface Widget { ... }

with a doc-comment but for @deprecated doc-comment tag (which is fine at the Slice level). What do you want to generate for the corresponding proxy in JS and TS?

Since JS/TS doesn't provide a deprecated attribute (as far as I can tell), generating an @deprecated JS/TS doc-comment seems better than not generating anything.

InsertCreativityHere added 2 commits October 31, 2024 13:09

Centralize comment-parsing code.

4bb8fb1

Remove unused parameters.

5d1740c

InsertCreativityHere commented Oct 31, 2024

View reviewed changes

Not sure why it built locally without 'include functional' but whatev…

06a9687

…er...

InsertCreativityHere commented Oct 31, 2024

View reviewed changes

InsertCreativityHere marked this pull request as ready for review October 31, 2024 17:59

InsertCreativityHere requested review from bernardnormier, pepone and externl and removed request for bernardnormier and pepone October 31, 2024 17:59

InsertCreativityHere added 2 commits October 31, 2024 14:08

Some platforms want us to qualify the 'replace' function.

eb2d8ca

Pull in the algorithm header. Still concerning this worked locally.

88a1aee

bernardnormier reviewed Oct 31, 2024

View reviewed changes

Removed logic for injecting deprecated metadata into doc comments.

0947315

InsertCreativityHere requested a review from bernardnormier October 31, 2024 19:08

InsertCreativityHere added 2 commits October 31, 2024 15:10

Formatting...

02c384b

Formatting... again...

3609630

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Centralize comment parsing #3030

Centralize comment parsing #3030

InsertCreativityHere commented Oct 31, 2024 •

edited

Loading

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

InsertCreativityHere Oct 31, 2024

bernardnormier left a comment

bernardnormier Oct 31, 2024 •

edited

Loading

InsertCreativityHere Oct 31, 2024

bernardnormier Oct 31, 2024

InsertCreativityHere Oct 31, 2024

bernardnormier Oct 31, 2024

InsertCreativityHere Oct 31, 2024

bernardnormier Oct 31, 2024

InsertCreativityHere Oct 31, 2024

bernardnormier Oct 31, 2024

InsertCreativityHere Oct 31, 2024

bernardnormier Oct 31, 2024

		// Fix any javadoc using the provided link formatter.
		const string link = "{@link ";

		/// Returns a doxygen formatted link to the provided Slice identifier.
		string cppLinkFormatter(string identifier) { return "{@link " + fixKwd(identifier) + "}"; }

Centralize comment parsing #3030

Are you sure you want to change the base?

Centralize comment parsing #3030

Conversation

InsertCreativityHere commented Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardnormier left a comment

Choose a reason for hiding this comment

bernardnormier Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

InsertCreativityHere commented Oct 31, 2024 •

edited

Loading

bernardnormier Oct 31, 2024 •

edited

Loading