Improve Doc-Comment Link Generation Machinery #3434

InsertCreativityHere · 2025-01-27T20:21:40Z

The main change in this PR is that the callback used to map @link tags has been given more context to work with.
It used to take: (string identifier, string member) (parsed from @link identifier#member).
Now it takes: (string rawLink, ContainedPtr source, SyntaxTreeBasePtr target).
Having pointers to the elements lets us do things like check for cpp:identifier.

This PR also removes our use of DocCommentPtr.
There was absolutely no reason to wrap DocComment in a shared_ptr, since it was never shared between anything.
Now we use optional<DocComment> (nullopt indicates there was no doc-comment on something).

And the function for parsing a doc-comment was changed from Contained::parseDocComment to DocComment::parseFrom.
This function was setting private fields on DocComment, so it makes sense to have it on DocComment and avoid a
friend Contained. This was also necessary to avoid some weird shared_from_this hackery.

bernardnormier

I anticipate a number of clang-tidy lints.

bernardnormier · 2025-01-27T22:46:34Z

cpp/src/ice2slice/Gen.cpp

-        // Replace links of the form `{@link Type#member}` with `{@link Type::member}`.
-        string result = "{@link ";
-        if (memberComponent.empty())
+        // The only difference with '@link' between Slice1 and Slice2 is that Slice1 uses '#' and Slice2 uses '::'.


I assume you mean the .ice and .slice syntax, not Slice1 and Slice2. Slice1 and Slice2 are two modes of the .slice syntax.

Are those the official terms we want to use for them?
The only difference with '@link' between the '.ice' and '.slice' syntaxes is that '.ice' syntax uses '#'?
Feels a little clunky to me.

We can also say Ice Slice files (.ice) and IceRPC Slice files (.slice) but is kind of verbose.

This is a comment in the ice2slice implementation so you could write:

The only difference with '@link' between the ice and slice syntaxes is that the ice syntax uses '#' ...

I went with 'Ice syntax' and 'Slice syntax' because I don't care enough to argue.
Of course, I'm still going to make my case though : vP

Because these terms feel pretty weird to me.
There is no such thing as 'Ice syntax' and 'Slice syntax' is ambiguous IMO.
Whereas 'Slice1' and 'Slice2' make it clear that we're only talking about the Slice syntax, but just two different versions of it.

It happens to be true that 'Slice1' and 'Slice2' are compilation modes in IceRPC, but that has no bearing here.
Ice has no compilation modes. And it's worth noting that the entire point of 'Slice1's existence is to interop with the 'Ice syntax'. So calling this 'Slice1' feels very natural to me.

InsertCreativityHere · 2025-01-27T23:41:37Z

I anticipate a number of clang-tidy lints.

Yes, I tried to keep up with all the lint-changes that were added, but I didn't read every PR in totality.
So, there's probably some that I'm unaware of.

I expect it to complain about passing my std::function by const &.
But given that I'm passing in a function pointer (and not a lambda), I really don't see the point of passing a reference.
So, I'm hoping it won't complain about this.

cpp/src/slice2cpp/Gen.cpp

pepone · 2025-01-28T14:42:46Z

cpp/src/slice2py/PythonUtil.cpp

@@ -1994,7 +2002,7 @@ Slice::Python::CodeVisitor::collectExceptionMembers(const ExceptionPtr& p, Membe
 }

 void
-Slice::Python::CodeVisitor::writeDocstring(const DocCommentPtr& comment, const string& prefix)
+Slice::Python::CodeVisitor::writeDocstring(optional<DocComment> comment, const string& prefix)


I think these methods should take a reference, as they are not taking ownership of the comment.

I changed it to use a const&.
I don't think it really matters though since we were always passing in xvalues to this function.

Co-authored-by: Jose <pepone@users.noreply.github.com>

InsertCreativityHere · 2025-01-27T22:03:53Z

cpp/src/Slice/Parser.cpp

@@ -692,39 +688,6 @@ namespace
            }
        }



The code for fixing @link tags was moved further down (and largely rewritten).

InsertCreativityHere · 2025-01-27T22:04:48Z

cpp/src/Slice/Parser.cpp

    {
-        doc.clear();


We leave it up to the caller to doc.clear() now.

InsertCreativityHere · 2025-01-27T22:06:12Z

cpp/src/Slice/Parser.cpp

+// I AM GOING TO MOVE THIS COMMENT PARSING UP THE FILE NEXT TO `DocComment`.
+// RIGHT NOW I'VE LEFT IT IN IT'S ORIGINAL PLACE, SO THE DIFF IS REVIEWABLE.


Once everyone has reviewed, I'll move all this code up next to DocComment where it belongs.

InsertCreativityHere · 2025-01-27T22:07:39Z

cpp/src/Slice/Parser.cpp

@@ -819,26 +842,29 @@ Slice::Contained::parseDocComment(
    const string returnTag = "@return";
    const string deprecatedTag = "@deprecated";



Now a plain DocComment instead of an unnecessary DocCommentPtr.

InsertCreativityHere · 2025-01-27T22:08:18Z

cpp/src/Slice/Parser.cpp

    string lineText;
    string name;

    // Parse the comment's text.
    for (const auto& l : lines)
    {
+        lineText.clear();


This is where we call clear now instead of inside parseCommentLine and parsseNamedCommentLine.

InsertCreativityHere · 2025-01-27T22:12:33Z

cpp/src/Slice/Parser.h

@@ -503,9 +516,9 @@ namespace Slice
            return false;
        }

-    protected:
        [[nodiscard]] std::string thisScope() const;


thisScope is used by DocComment::parseFrom now (in the process of resolving links).

InsertCreativityHere · 2025-01-27T23:38:42Z

cpp/src/ice2slice/Gen.cpp

-        // Replace links of the form `{@link Type#member}` with `{@link Type::member}`.
-        string result = "{@link ";
-        if (memberComponent.empty())
+        // The only difference with '@link' between Slice1 and Slice2 is that Slice1 uses '#' and Slice2 uses '::'.


Are those the official terms we want to use for them?
The only difference with '@link' between the '.ice' and '.slice' syntaxes is that '.ice' syntax uses '#'?
Feels a little clunky to me.

…matting-callback

InsertCreativityHere added 12 commits January 27, 2025 10:42

Minor comment-parsing simplifications.

b528a11

String views.

4dd344d

Move the link formatting into the main 'parseDocComment' function.

6e53377

Switched the core of the parser code.

9df5de3

Forgot 'std' prefix, and uninit bool...

4d48f3a

Finished everything else.

702f489

Formatting.

0af290a

Self-review checks.

cca7bbd

More self-review improvements.

750a22f

Fix dead-link warnings.

db04f9b

Merge branch 'main' into improve-link-formatting-callback

f20a43a

Added more comments.

371e37f

InsertCreativityHere requested review from bernardnormier and pepone January 27, 2025 22:32

InsertCreativityHere marked this pull request as ready for review January 27, 2025 22:32

bernardnormier reviewed Jan 27, 2025

View reviewed changes

pepone approved these changes Jan 28, 2025

View reviewed changes

Fix typo.

99e638f

Co-authored-by: Jose <pepone@users.noreply.github.com>

InsertCreativityHere commented Jan 28, 2025

View reviewed changes

Fix lints (probably).

fbd6362

InsertCreativityHere requested a review from bernardnormier January 29, 2025 20:12

InsertCreativityHere added 6 commits January 29, 2025 15:29

Revert the Java Gen files to make merge conflicts easier to fix.

161c543

Merge branch 'main' of github.com:zeroc-ice/ice into improve-link-for…

5ee3a2c

…matting-callback

I think I replayed my changes on top of main okay...

c1e5889

Changed some API to use a const&.

9765e54

Final things.

712ebe4

Formatting... again...

cf115d6

bernardnormier approved these changes Jan 29, 2025

View reviewed changes

Move the code up to DocComment like I said.

acee118

InsertCreativityHere merged commit f07b487 into zeroc-ice:main Jan 30, 2025
20 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Doc-Comment Link Generation Machinery #3434

Improve Doc-Comment Link Generation Machinery #3434

InsertCreativityHere commented Jan 27, 2025 •

edited

Loading

bernardnormier left a comment

bernardnormier Jan 27, 2025

InsertCreativityHere Jan 27, 2025

pepone Jan 28, 2025

bernardnormier Jan 28, 2025

InsertCreativityHere Jan 29, 2025

InsertCreativityHere commented Jan 27, 2025

pepone Jan 28, 2025

InsertCreativityHere Jan 29, 2025

InsertCreativityHere Jan 27, 2025

InsertCreativityHere Jan 27, 2025

InsertCreativityHere Jan 27, 2025

InsertCreativityHere Jan 27, 2025

InsertCreativityHere Jan 27, 2025

InsertCreativityHere Jan 27, 2025

InsertCreativityHere Jan 27, 2025

		// I AM GOING TO MOVE THIS COMMENT PARSING UP THE FILE NEXT TO `DocComment`.
		// RIGHT NOW I'VE LEFT IT IN IT'S ORIGINAL PLACE, SO THE DIFF IS REVIEWABLE.

		@@ -819,26 +842,29 @@ Slice::Contained::parseDocComment(
		const string returnTag = "@return";
		const string deprecatedTag = "@deprecated";

Improve Doc-Comment Link Generation Machinery #3434

Improve Doc-Comment Link Generation Machinery #3434

Conversation

InsertCreativityHere commented Jan 27, 2025 • edited Loading

bernardnormier left a 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 Jan 27, 2025

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 Jan 27, 2025 •

edited

Loading