Review and Fix All Slice Doc-Comments #4350

InsertCreativityHere · 2025-08-22T15:21:58Z

I manually reviewed every single doc-comment in every Slice file.
Originally I was just fixing #4329 (which this PR does), by adding links to places where AUTOLINK used to.

But I also came across lots of bad formatting, incorrect comments, and bad grammar.
So I ended up just fixing everything.

I tried using Copilot on the larger files (Contract.ice, Admin.ice), but it goes too crazy rewriting sentences instead of just fixing objective mistakes. Unfortunate.

InsertCreativityHere · 2025-08-22T15:48:12Z

cpp/src/IceGrid/Internal.ice


 module IceGrid
 {
    // This class is no longer used. We keep it only for interop with IceGrid 3.7.


This comment says the class is only for interop, and it's not even a doc-comment.
So I marked this deprecated to signal that it shouldn't be used for new projects.

Is this fine? Or should it be left un-deprecated so users don't see deprecation warnings?

You're editing Internal.ice. It's not a public file.

InsertCreativityHere · 2025-08-22T15:48:50Z

cpp/src/IceGrid/Internal.ice

        InternalAdapterDescriptorSeq adapters;

        // Not used, always empty. Kept only for interop with IceGrid 3.7.
-        InternalDbEnvDescriptorSeq dbEnvs;


Same idea: should this stay undeprecated even if it's never used anymore?

Same feedback: since it's Internal, the deprecated is only going to affect internal code. You may need to update some code to turn-off these warnings.

InsertCreativityHere · 2025-08-22T15:49:58Z

cpp/src/IceGrid/Internal.ice

+        /// Activate this adapter. If this adapter can be activated, this will activate the adapter and return the
+        /// direct proxy of the adapter once it's active. If this adapter can be activated on demand, this will return
+        /// `null` if the adapter is inactive or the adapter direct proxy it's active.


These lines were over the 120 column limit.

this will return 0 is impossible. I'm just hoping we meant null here.

InsertCreativityHere · 2025-08-22T15:50:46Z

cpp/src/IceGrid/Internal.ice

+        /// @param proxy The direct proxy.
+        /// The direct proxy should be created with the object adapter and should contain the object adapter endpoints.


Also over the 120 column limit.

InsertCreativityHere · 2025-08-22T15:51:18Z

cpp/src/IceGrid/Internal.ice

    interface Node extends FileReader, ReplicaObserver
    {
        /// Load the given server. If the server resources weren't already created (database environment directories,
-            /// property files, etc), they will be created. The returned proxy is never null.


No idea where there's extra indentation here.
Removed.

InsertCreativityHere · 2025-08-22T15:56:30Z

cpp/src/IceStorm/Election.ice


        /// Called to accept an invitation into the given group.
        /// @param j The id of the node accepting the invitation.
-        /// @param observer The observer.


Also out of order here.

InsertCreativityHere · 2025-08-22T15:56:53Z

cpp/src/IceStorm/IceStormInternal.ice

+        /// @return The replica proxy, or `null` if this instance is not replicated.
        ["cpp:const"] idempotent IceStormElection::Node* getReplicaNode();
    }
-} // End module IceStorm


Why even comment this?
Deleted.

It's common in C++, especially when using the same indentation level for several constructs (namespace, class).

Now that we use a nicer indentation for Slice, it's not helpful

InsertCreativityHere · 2025-08-22T15:57:24Z

doxygen/Doxyfile

 # globally by setting AUTOLINK_SUPPORT to NO.
 # The default value is: YES.

-AUTOLINK_SUPPORT       = YES


Disable AUTOLINKing in Slice.

InsertCreativityHere · 2025-08-22T15:58:18Z

js/test/Ice/operations/Test.ice


 module Test2
 {
-    /**


This shouldn't be a doc-comment.
It's /* in ever of language's operation/Test.ice file.

InsertCreativityHere · 2025-08-22T15:58:46Z

slice/Glacier2/Router.ice

        /// If a non-null session proxy is returned, it must be configured to route through the router that created it.
        /// This occurs automatically when the router is configured as the client's default router at the time the
        /// session proxy is created in the client application; otherwise, the client must configure the session proxy
        /// explicitly.


Normally we place @see after the operation tags.

InsertCreativityHere · 2025-08-22T16:06:49Z

slice/IceGrid/Exception.ice

        string reason;
    }

-    /// The exception that is thrown when a server failed to start.


This was a bad copy-paste from ServerStartException directly above here.

Copilot

Pull Request Overview

This PR comprehensively reviews and fixes all doc-comments in Slice files across the codebase. The primary purpose is to address issue #4329 by adding proper links where AUTOLINK was previously used, while also correcting formatting errors, grammatical mistakes, and inconsistent documentation throughout.

Key changes include:

Replacement of implicit references with proper {@link} syntax for cross-references
Correction of parameter references using @p notation
Fix of grammatical errors and formatting inconsistencies

Reviewed Changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
swift/test/Ice/hold/Test.ice	Updated parameter references and method names in code blocks
slice/IceStorm/IceStorm.ice	Added link formatting and corrected parameter references
slice/IceLocatorDiscovery/Lookup.ice	Enhanced cross-references with proper link syntax
slice/IceGrid/Session.ice	Improved parameter references and cross-links
slice/IceGrid/Registry.ice	Minor wording improvements for consistency
slice/IceGrid/FileParser.ice	Enhanced cross-reference formatting
slice/IceGrid/Exception.ice	Fixed incorrect exception description
slice/IceGrid/Descriptor.ice	Added sequence type links and formatting fixes
slice/IceGrid/Admin.ice	Improved formatting and cross-references
slice/IceDiscovery/Lookup.ice	Enhanced interface references with proper links
slice/Ice/Router.ice	Grammar and formatting improvements
slice/Ice/RemoteLogger.ice	Enhanced cross-references and parameter formatting
slice/Ice/Metrics.ice	Corrected pluralization and parameter references
slice/Ice/LocatorRegistry.ice	Enhanced cross-references and parameter formatting
slice/Ice/Locator.ice	Removed redundant link formatting
slice/Ice/Context.ice	Minor grammar correction
slice/Glacier2/Session.ice	Grammar fixes and enhanced cross-references
slice/Glacier2/Router.ice	Reorganized see-also references
slice/Glacier2/PermissionsVerifier.ice	Grammar correction
js/test/Ice/operations/Test.ice	Changed multi-line to single-line comment
java/test/src/main/java/test/Ice/location/Test.ice	Pluralization fix
java/test/src/main/java/test/Ice/hold/Test.ice	Parameter and method reference improvements
doxygen/Doxyfile	Disabled AUTOLINK_SUPPORT
csharp/test/Ice/hold/Test.ice	Parameter and method reference improvements
cpp/test/Ice/hold/Test.ice	Parameter and method reference improvements
cpp/src/IceStorm/LinkRecord.ice	Enhanced cross-references
cpp/src/IceStorm/IceStormInternal.ice	Comprehensive formatting and reference improvements
cpp/src/IceStorm/Election.ice	Parameter reordering and formatting fixes
cpp/src/IceGrid/Internal.ice	Extensive formatting and reference improvements
cpp/src/DataStorm/Contract.ice	Enhanced cross-references and formatting
cpp/include/Ice/ObjectAdapter.h	Comment formatting fix
cpp/include/DataStorm/Node.h	Comment formatting fix

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-08-22T16:09:02Z

slice/IceGrid/Exception.ice


-    /// The exception that is thrown when a server failed to start.
+    /// The exception that is thrown when a server failed to stop.
    exception ServerStopException


The comment describes an exception for when a server failed to stop, but the exception name is ServerStopException. This is contradictory - if this exception is thrown when a server failed to stop, it should be named something like ServerStopFailedException. Either the comment or the exception name is incorrect.

Suggested change

exception ServerStopException

exception ServerStopFailedException

bernardnormier · 2025-08-22T21:05:47Z

cpp/src/DataStorm/Contract.ice

    interface SubscriberSession extends Session
    {
-        /// Queue a sample with the subscribers of the topic element.
+        /// Queue a @p sample with the subscribers of the topic element.


I think this should be either "queue @p sample" or "queue a sample", not a mix of both.

Also, all these purely internal doc-comments aren't that important.

bernardnormier · 2025-08-22T21:06:39Z

cpp/src/DataStorm/Contract.ice

    /// The lookup interface is used by DataStorm nodes to announce their topic readers and writers to other connected
    /// nodes. When multicast is enabled, the lookup interface also broadcasts these announcements.
-    /// Each DataStorm node hosts a lookup servant with the identity "DataStorm/Lookup".
+    /// Each DataStorm node hosts a lookup servant with the identity `'DataStorm/Lookup'`.


A code span without quotes is sufficient.

Alright, I updated all our identity examples to just use a code-span with nothing else.

bernardnormier · 2025-08-22T21:08:07Z

cpp/src/IceGrid/Internal.ice


 module IceGrid
 {
    // This class is no longer used. We keep it only for interop with IceGrid 3.7.


You're editing Internal.ice. It's not a public file.

bernardnormier · 2025-08-22T21:10:22Z

cpp/src/IceGrid/Internal.ice

        InternalAdapterDescriptorSeq adapters;

        // Not used, always empty. Kept only for interop with IceGrid 3.7.
-        InternalDbEnvDescriptorSeq dbEnvs;


Same feedback: since it's Internal, the deprecated is only going to affect internal code. You may need to update some code to turn-off these warnings.

bernardnormier · 2025-08-22T21:14:20Z

cpp/src/IceStorm/Election.ice

-        /// @param gn The group name.
        /// @param j The group coordinator.
+        /// @param gn The group name.
        void invitation(int j, string gn);


And Slice compiler did not issue a warning?

bernardnormier · 2025-08-22T21:16:00Z

cpp/src/IceStorm/IceStormInternal.ice

+        /// @return The replica proxy, or `null` if this instance is not replicated.
        ["cpp:const"] idempotent IceStormElection::Node* getReplicaNode();
    }
-} // End module IceStorm


It's common in C++, especially when using the same indentation level for several constructs (namespace, class).

Now that we use a nicer indentation for Slice, it's not helpful

bernardnormier · 2025-08-22T21:19:46Z

slice/Ice/LocatorRegistry.ice

+        /// @param proxy A proxy to the {@link Process} object of the server. This proxy is never null.
        /// @throws ServerNotFoundException Thrown when the locator does not know a server application with this server
-        /// ID.
+        /// @p id.


should be "with server ID @p id".

Fixed. I see what you mean.

bernardnormier · 2025-08-22T21:21:01Z

slice/Ice/Metrics.ice

            throws UnknownMetricsView;

-        /// Gets the metrics objects for the given metrics view.
+        /// Gets the metrics objects for the given metrics @p view.


Should be: "... for the metrics view @p view"

Fixed. Felt more natural to remove the @p here.

bernardnormier · 2025-08-22T21:22:01Z

slice/Ice/Metrics.ice

            throws UnknownMetricsView;

-        /// Gets the metrics failures associated with the given view and map.
+        /// Gets the metrics failures associated with the given @p view and @p map.


I'd remove "given". You mark them as parameters, they are clearly "given".

We use 'given' all the time in our Slice files, so I'm going to leave it for now to get this merged.
Can always re-review all our comments again in the future.

bernardnormier · 2025-08-22T21:23:15Z

slice/Ice/RemoteLogger.ice

    interface LoggerAdmin
    {
-        /// Attaches a RemoteLogger object to the local logger. This operation calls {@link RemoteLogger#init} on @p prx.
+        /// Attaches a {@link RemoteLogger} object to the local logger. This operation calls {@link RemoteLogger#init}


I thought operation-referencing syntax was "::", not "#".

You use both in this PR. Are they both correct?

Doxygen supports # anywhere where :: can appear but not the other way.
So, this does work, but we really should use :: for operations. This one just slipped by me.

Fixed.

InsertCreativityHere added 3 commits August 22, 2025 10:35

Review and fix all Slice doc-comments.

1ae8e7b

Self-review fixes.

b3ca102

More fixes.

f679b66

InsertCreativityHere commented Aug 22, 2025

View reviewed changes

InsertCreativityHere marked this pull request as ready for review August 22, 2025 16:07

InsertCreativityHere requested review from Copilot and bernardnormier August 22, 2025 16:08

Copilot AI reviewed Aug 22, 2025

View reviewed changes

bernardnormier reviewed Aug 22, 2025

View reviewed changes

Review fixes.

715c24d

bernardnormier self-requested a review August 25, 2025 15:24

bernardnormier approved these changes Aug 25, 2025

View reviewed changes

InsertCreativityHere merged commit 7337ce1 into zeroc-ice:main Aug 25, 2025
26 of 28 checks passed

InsertCreativityHere mentioned this pull request Aug 25, 2025

Slice doc-comments and autolinking #4329

Closed

pepone pushed a commit to pepone/ice that referenced this pull request Sep 16, 2025

Review and Fix All Slice Doc-Comments (zeroc-ice#4350)

76f3079

		/// @param proxy The direct proxy.
		/// The direct proxy should be created with the object adapter and should contain the object adapter endpoints.

	exception ServerStopException
	exception ServerStopFailedException

Review and Fix All Slice Doc-Comments #4350

Review and Fix All Slice Doc-Comments #4350

Uh oh!

Conversation

InsertCreativityHere commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

InsertCreativityHere commented Aug 22, 2025 •

edited

Loading